2.0 proposal

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

2.0 proposal

James Nord-3
Hi all,

So we have strived to keep Jenkins backwards compatible and Kohsuke did want to try and keep backwards compatibility where possible.

But I have a proposal I would like to table that will break this as the current situation is not sustainable in my opinion.

All non-private fields (except public static finals) in the core code shall be removed (replaced with getters/setters) - and there shall not be any use of the bytecode-compatibility-transformer and @AdaptField.

Why?

  1. the use of fields breaks encapsulation and makes maintaining the code and enforcing correct locking harder (why we need to remove them)
  2. We are now in the realms where the bytecode compatability transformer is causing issues (it has been for a while but...) (why we need to stop using the bct)
    1. the rewriting now requires to load classes which they themselves may need to be re-written and is brittle (currently this just blows up but for JDK6+ classes this will become a much more prevelant issue)
    2. the transformer rewrites classes that do not need re-writing.  In order to do this correctly we need to traverse the entire class hierarchy due to the format of the bytecode - and this also is brittle (potentially loading classes whilst loading classes - which will cause further issues).
    3. I can see the above blowing up at some point with some funky circular reference which is perfectly legal

Infact having just fixed JENKINS-30820, we now see broken jruby runtime plugin with a ClassCircularityError.

https://issues.jenkins-ci.org/browse/JENKINS-19383  (not fixed  just hidden - still rewrites this class - hence the ticket below)

Post JDK6 the bytecode is now much more complex - to the point that until jdk 1.8 update 60 (just released) the jvm bytecode verifier was letting invalid bytecode past the verifier - which has suddenly showed some more broken transformations prior to JENKINS-28781 landing in 1.633 - and who knows what JDK9 will bring in terms of new op-codes or requirements.

/James

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/54f53a29-4ce1-4582-830a-78c0c2274de3%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: 2.0 proposal

stephenconnolly
+1000 (having had to dance around the @AdaptField on private fields
called "id" in Jenkins 1.602+ more times than I'd care to recall
already this year)

On 19 October 2015 at 14:13, James Nord <[hidden email]> wrote:

> Hi all,
>
> So we have strived to keep Jenkins backwards compatible and Kohsuke did want
> to try and keep backwards compatibility where possible.
>
> But I have a proposal I would like to table that will break this as the
> current situation is not sustainable in my opinion.
>
> All non-private fields (except public static finals) in the core code shall
> be removed (replaced with getters/setters) - and there shall not be any use
> of the bytecode-compatibility-transformer and @AdaptField.
>
> Why?
>
> the use of fields breaks encapsulation and makes maintaining the code and
> enforcing correct locking harder (why we need to remove them)
> We are now in the realms where the bytecode compatability transformer is
> causing issues (it has been for a while but...) (why we need to stop using
> the bct)
>
> the rewriting now requires to load classes which they themselves may need to
> be re-written and is brittle (currently this just blows up but for JDK6+
> classes this will become a much more prevelant issue)
> the transformer rewrites classes that do not need re-writing.  In order to
> do this correctly we need to traverse the entire class hierarchy due to the
> format of the bytecode - and this also is brittle (potentially loading
> classes whilst loading classes - which will cause further issues).
> I can see the above blowing up at some point with some funky circular
> reference which is perfectly legal
>
>
> Infact having just fixed JENKINS-30820, we now see a broken jruby runtime
> plugin with a ClassCircularityError.
>
> https://issues.jenkins-ci.org/browse/JENKINS-28781
> https://issues.jenkins-ci.org/browse/JENKINS-30820
> https://issues.jenkins-ci.org/browse/JENKINS-19383  (not fixed  just hidden
> - still rewrites this class - hence the ticket below)
> https://issues.jenkins-ci.org/browse/JENKINS-28799  (not fixed)
> https://issues.jenkins-ci.org/browse/JENKINS-31019
>
> Post JDK6 the bytecode is now much more complex - to the point that until
> jdk 1.8 update 60 (just released) the jvm bytecode verifier was letting
> invalid bytecode past the verifier - which has suddenly showed some more
> broken transformations prior to JENKINS-28781 landing in 1.633 - and who
> knows what JDK9 will bring in terms of new op-codes or requirements.
>
> /James
>
> --
> You received this message because you are subscribed to the Google Groups
> "Jenkins Developers" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to [hidden email].
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/jenkinsci-dev/54f53a29-4ce1-4582-830a-78c0c2274de3%40googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CA%2BnPnMz7QCTCZO-8f13oM0RqMjYSoHXr1wmjbGuqEUMOc4WHQw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: 2.0 proposal

Jesse Glick-4
In reply to this post by James Nord-3
On Mon, Oct 19, 2015 at 9:13 AM, James Nord <[hidden email]> wrote:
> there shall not be any use of the bytecode-compatibility-transformer and @AdaptField.

I see your point; this looks very scary, and we clearly do not have
any JVM experts on hand to design a system that is truly foolproof.
(Or sufficiently powerful—`@AdaptField` only helps with a very limited
set of problems, and if we want to continue splitting plugins from
core¹ we need a lot more.) On the other hand we are looking at the
potential for a *lot* of `LinkageError`s hitting innocent users during
an upgrade.

I wonder if we can find some more static, debuggable, easily
understood procedure for solving this class of problem. For example

1. Deprecate things we do not want people using and introduce
replacements. (As soon as possible—not waiting for 2.0.)
2. Provide a precise, perhaps mechanically enforceable refactoring.
NetBeans declarative refactorings² are better than nothing, though I
would like to see a batch-mode tool (+ Maven plugin) to apply them.
3. Enhance `plugin-compat-tester` to check for deprecation warnings
when compiling against the newest dependency, or otherwise find usages
of members we propose to delete. Have a team responsible for finding &
fixing failures, if necessary incrementing the plugin’s baseline
version to get access to the replacement.
4. Introduce a facility to the Jenkins plugin/update system allowing
us to declare that a specific version of the plugin is the first known
to be compatible with a specific version of a (core or plugin)
dependency. If you attempt to upgrade the dependency you will be
blocked unless you also agree to update the dependent.


¹https://issues.jenkins-ci.org/issues/?jql=resolution%20%3D%20Unresolved%20and%20labels%20in%20(split-plugins-from-core)
²Example: https://github.com/jenkinsci/jenkins/blob/3d2956d9ad65e34cc74949e5b397b7475c6cb04b/core/src/main/resources/META-INF/upgrade/AbstractTestResultAction.hint

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr105YojNuJrpOt2a3sCWrAVbSnLoMY3aF-SM312M2_0_A%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: 2.0 proposal

ogondza
In reply to this post by James Nord-3
That all makes sense, though to implement that we need a policy to remove  
deprecated API first. Removing non-private fields should be done by  
deprecating it first.

I am tracking [1] for this effort. I would like to have a look at early  
deprecated API use detection.

[1] https://issues.jenkins-ci.org/browse/JENKINS-31035

--
oliver

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/op.x6rlt6e0sbfict%40arch.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: 2.0 proposal

Kohsuke Kawaguchi
Administrator
In reply to this post by James Nord-3
The part about proactively changing non-private fields is too reckless. It presumably breaks a significant number of plugins, and it does so for no good reasons. I say no good reasons because most of these fields need not be adopted ever.

IIUC, the pain that motivated you is the side effect of bytecode compatibility transformer, so what we should be thinking about is how to minimize the use & impact of BCT.

For example, Queue.Item.id has known fixed set of subtypes, so we can restrict the rewrite to a much smaller subset. And if we want to discourage the use of BCT, we can do that, too.


2015-10-19 6:13 GMT-07:00 James Nord <[hidden email]>:
Hi all,

So we have strived to keep Jenkins backwards compatible and Kohsuke did want to try and keep backwards compatibility where possible.

But I have a proposal I would like to table that will break this as the current situation is not sustainable in my opinion.

All non-private fields (except public static finals) in the core code shall be removed (replaced with getters/setters) - and there shall not be any use of the bytecode-compatibility-transformer and @AdaptField.

Why?

  1. the use of fields breaks encapsulation and makes maintaining the code and enforcing correct locking harder (why we need to remove them)
  2. We are now in the realms where the bytecode compatability transformer is causing issues (it has been for a while but...) (why we need to stop using the bct)
    1. the rewriting now requires to load classes which they themselves may need to be re-written and is brittle (currently this just blows up but for JDK6+ classes this will become a much more prevelant issue)
    2. the transformer rewrites classes that do not need re-writing.  In order to do this correctly we need to traverse the entire class hierarchy due to the format of the bytecode - and this also is brittle (potentially loading classes whilst loading classes - which will cause further issues).
    3. I can see the above blowing up at some point with some funky circular reference which is perfectly legal

Infact having just fixed JENKINS-30820, we now see broken jruby runtime plugin with a ClassCircularityError.

https://issues.jenkins-ci.org/browse/JENKINS-19383  (not fixed  just hidden - still rewrites this class - hence the ticket below)

Post JDK6 the bytecode is now much more complex - to the point that until jdk 1.8 update 60 (just released) the jvm bytecode verifier was letting invalid bytecode past the verifier - which has suddenly showed some more broken transformations prior to JENKINS-28781 landing in 1.633 - and who knows what JDK9 will bring in terms of new op-codes or requirements.

/James

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/54f53a29-4ce1-4582-830a-78c0c2274de3%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.



--
Kohsuke Kawaguchi

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAN4CQ4zGbxtAZXNJSGnm3wGmj_jwob1ShYL-jcVvJN%2BgT%2BKCFg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: 2.0 proposal

Jesse Glick-4
On Mon, Oct 19, 2015 at 2:19 PM, Kohsuke Kawaguchi <[hidden email]> wrote:
> Queue.Item.id has known fixed set of subtypes, so we can restrict the rewrite to a much smaller subset.

Not sure I follow that. IIUC the problem is outside references to
`id`. The number of subtypes of `Item` (and the fact that they are all
in core) is irrelevant.

> if we want to discourage the use of BCT, we can do that, too.

I think to gain the benefits it has to be completely removed, not just
“discouraged”.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr26od5cr7wjXzT5UfFkNACDTM6dfZDxZSrHhc%3DrrAiQyg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: 2.0 proposal

James Nord-3


On Monday, October 19, 2015 at 9:19:44 PM UTC+2, Jesse Glick wrote:
On Mon, Oct 19, 2015 at 2:19 PM, Kohsuke Kawaguchi <<a href="javascript:" target="_blank" gdf-obfuscated-mailto="d8ZCM2UcCwAJ" rel="nofollow" onmousedown="this.href=&#39;javascript:&#39;;return true;" onclick="this.href=&#39;javascript:&#39;;return true;">k...@...> wrote:
> <a href="http://Queue.Item.id" target="_blank" rel="nofollow" onmousedown="this.href=&#39;http://www.google.com/url?q\75http%3A%2F%2FQueue.Item.id\46sa\75D\46sntz\0751\46usg\75AFQjCNHi6tu0RnP2rk3tyY1-X3dTIfPr1g&#39;;return true;" onclick="this.href=&#39;http://www.google.com/url?q\75http%3A%2F%2FQueue.Item.id\46sa\75D\46sntz\0751\46usg\75AFQjCNHi6tu0RnP2rk3tyY1-X3dTIfPr1g&#39;;return true;">Queue.Item.id has known fixed set of subtypes, so we can restrict the rewrite to a much smaller subset.

Not sure I follow that. IIUC the problem is outside references to
`id`. The number of subtypes of `Item` (and the fact that they are all
in core) is irrelevant.

All we know is that class X has a field called `id`
In order to see if it is truly Item.id being referenced you need to load all the subclasses of X and walk them (we are back to loading subclassess in the classloader - eek)
https://github.com/jenkinsci/bytecode-compatibility-transformer/blob/master/src/main/java/org/jenkinsci/bytecode/TransformationSpec.java#L23-L30



> if we want to discourage the use of BCT, we can do that, too.

I think to gain the benefits it has to be completely removed, not just
“discouraged”.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/bf6ca3e8-be3e-49a4-8faa-4c6b3ee4d0ea%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: 2.0 proposal

Kohsuke Kawaguchi
Administrator
In reply to this post by Jesse Glick-4


2015-10-19 12:19 GMT-07:00 Jesse Glick <[hidden email]>:
On Mon, Oct 19, 2015 at 2:19 PM, Kohsuke Kawaguchi <[hidden email]> wrote:
> Queue.Item.id has known fixed set of subtypes, so we can restrict the rewrite to a much smaller subset.

Not sure I follow that. IIUC the problem is outside references to
`id`. The number of subtypes of `Item` (and the fact that they are all
in core) is irrelevant.

The only reference to 'id' that we need to rewrite is those for Queue.Item and its subtypes. Today BCT doesn't assume that the subtypes are finite fixed set, so it rewrites every reference to com.example.Foo#id based on the assumption that it might be a subtype of Queue.Item.

By knowing all the subtypes, we can drastically reduces the amount of rewrite.

--
Kohsuke Kawaguchi

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAN4CQ4wv%2BYLbKNbD4a5Y0jeWkrns_pmcDhtq9XvbdQgXLXZKpw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: 2.0 proposal

James Nord-3


On Tuesday, October 20, 2015 at 12:58:30 AM UTC+2, Kohsuke Kawaguchi wrote:


2015-10-19 12:19 GMT-07:00 Jesse Glick <<a href="javascript:" target="_blank" gdf-obfuscated-mailto="KwGYUlUoCwAJ" rel="nofollow" onmousedown="this.href=&#39;javascript:&#39;;return true;" onclick="this.href=&#39;javascript:&#39;;return true;">jgl...@...>:
On Mon, Oct 19, 2015 at 2:19 PM, Kohsuke Kawaguchi <<a href="javascript:" target="_blank" gdf-obfuscated-mailto="KwGYUlUoCwAJ" rel="nofollow" onmousedown="this.href=&#39;javascript:&#39;;return true;" onclick="this.href=&#39;javascript:&#39;;return true;">k...@...> wrote:
> <a href="http://Queue.Item.id" rel="nofollow" target="_blank" onmousedown="this.href=&#39;http://www.google.com/url?q\75http%3A%2F%2FQueue.Item.id\46sa\75D\46sntz\0751\46usg\75AFQjCNHi6tu0RnP2rk3tyY1-X3dTIfPr1g&#39;;return true;" onclick="this.href=&#39;http://www.google.com/url?q\75http%3A%2F%2FQueue.Item.id\46sa\75D\46sntz\0751\46usg\75AFQjCNHi6tu0RnP2rk3tyY1-X3dTIfPr1g&#39;;return true;">Queue.Item.id has known fixed set of subtypes, so we can restrict the rewrite to a much smaller subset.

Not sure I follow that. IIUC the problem is outside references to
`id`. The number of subtypes of `Item` (and the fact that they are all
in core) is irrelevant.

The only reference to 'id' that we need to rewrite is those for Queue.Item and its subtypes. Today BCT doesn't assume that the subtypes are finite fixed set, so it rewrites every reference to com.example.Foo#id based on the assumption that it might be a subtype of Queue.Item.

By knowing all the subtypes, we can drastically reduces the amount of rewrite.


We don't know the subtypes at compile time - nor at runtime whilst the classes are being loaded - without loading more classes (or at least loading the bytecode for those classes and inspecting said bytecode)

Also it's not the subtypes that need rewriting - it's the accessors of the field in other classes need to be re-written to use the new methods - (JENKINS-28799) - to reduce scope but it is just a sticky-plaster that reduces the amount of incorrect bytecode we may create.

Lets for argument sake say we did need to re-write a field in the ruby plugin - would/could we still not hit something like JENKINS-31019?

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/18263ca4-502e-4a91-83ff-163e0babadb3%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: 2.0 proposal

Kohsuke Kawaguchi
Administrator


2015-10-20 2:53 GMT-07:00 James Nord <[hidden email]>:


On Tuesday, October 20, 2015 at 12:58:30 AM UTC+2, Kohsuke Kawaguchi wrote:


2015-10-19 12:19 GMT-07:00 Jesse Glick <[hidden email]>:
On Mon, Oct 19, 2015 at 2:19 PM, Kohsuke Kawaguchi <[hidden email]> wrote:
> Queue.Item.id has known fixed set of subtypes, so we can restrict the rewrite to a much smaller subset.

Not sure I follow that. IIUC the problem is outside references to
`id`. The number of subtypes of `Item` (and the fact that they are all
in core) is irrelevant.

The only reference to 'id' that we need to rewrite is those for Queue.Item and its subtypes. Today BCT doesn't assume that the subtypes are finite fixed set, so it rewrites every reference to com.example.Foo#id based on the assumption that it might be a subtype of Queue.Item.

By knowing all the subtypes, we can drastically reduces the amount of rewrite.


We don't know the subtypes at compile time - nor at runtime whilst the classes are being loaded - without loading more classes (or at least loading the bytecode for those classes and inspecting said bytecode)

I was thinking about having a human statically listing them.

Also it's not the subtypes that need rewriting - it's the accessors of the field in other classes need to be re-written to use the new methods - (JENKINS-28799) - to reduce scope but it is just a sticky-plaster that reduces the amount of incorrect bytecode we may create.

I've heard this BCT issue described as "whack a mole", from which I assumed that the impact is proportional to the use of this feature, in which case reducing the amount of rewrite would help considerably.

But as I look more into it, it feels less and less like that. The picture I'm getting now is that JENKINS-28799 is about BCT not updating StackMapFrame properly, and our attempt to fix it introduced other problems like JENKINS-31019.

If those are the only issue, I still think the overall bytecode transformation strategy in BCT is sound. Given that stack map frame recomputation requires classloading, my suggestion is to tweak the transformation so that it doesn't require adjusting stack frame map. I think this is relatively easily achievable by moving the transformed code into a separate method so that bytecode index remain the same.

That is, whereas today the transformation is as follows:

    ... bunch of bytecode ...
    GETFIELD com.example.Foo#id
    ... bunch of bytecode ...

          => 

    ... bunch of bytecode ...
    if ( LHS instanceof TYPE ) {
         INVOKEVIRTUAL com.example.Foo#getId()
    } else {
         GETFIELD com.example.Foo#id
    }
    ... bunch of bytecode ...

Replace this with:

    ... bunch of bytecode ...
    INVOKSTATIC helperMethodWeAdd(Foo)
    ... bunch of bytecode ...

... where the helper method is the if/else block. The stack map analysis of the helper method does not contain any joinining of flow, so there's no need to compute the common base type, and hence it shouldn't require any classloading.

(Alternatively, I think it is also possible to just update existing stack frame map incrementally by inserting additional intermediate frames.)

--
Kohsuke Kawaguchi

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAN4CQ4zidKNqzy-7HJhbEEj2A_jU1zK5k75hapCG-QDScQhHzA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: 2.0 proposal

stephenconnolly
/me suspects somebody doesn't want to give up their golden hammer ;-)

On 20 October 2015 at 19:38, Kohsuke Kawaguchi <[hidden email]> wrote:

>
>
> 2015-10-20 2:53 GMT-07:00 James Nord <[hidden email]>:
>>
>>
>>
>> On Tuesday, October 20, 2015 at 12:58:30 AM UTC+2, Kohsuke Kawaguchi
>> wrote:
>>>
>>>
>>>
>>> 2015-10-19 12:19 GMT-07:00 Jesse Glick <[hidden email]>:
>>>>
>>>> On Mon, Oct 19, 2015 at 2:19 PM, Kohsuke Kawaguchi <[hidden email]>
>>>> wrote:
>>>> > Queue.Item.id has known fixed set of subtypes, so we can restrict the
>>>> > rewrite to a much smaller subset.
>>>>
>>>> Not sure I follow that. IIUC the problem is outside references to
>>>> `id`. The number of subtypes of `Item` (and the fact that they are all
>>>> in core) is irrelevant.
>>>
>>>
>>> The only reference to 'id' that we need to rewrite is those for
>>> Queue.Item and its subtypes. Today BCT doesn't assume that the subtypes are
>>> finite fixed set, so it rewrites every reference to com.example.Foo#id based
>>> on the assumption that it might be a subtype of Queue.Item.
>>>
>>> By knowing all the subtypes, we can drastically reduces the amount of
>>> rewrite.
>>>
>>
>> We don't know the subtypes at compile time - nor at runtime whilst the
>> classes are being loaded - without loading more classes (or at least loading
>> the bytecode for those classes and inspecting said bytecode)
>
>
> I was thinking about having a human statically listing them.
>
>> Also it's not the subtypes that need rewriting - it's the accessors of the
>> field in other classes need to be re-written to use the new methods -
>> (JENKINS-28799) - to reduce scope but it is just a sticky-plaster that
>> reduces the amount of incorrect bytecode we may create.
>
>
> I've heard this BCT issue described as "whack a mole", from which I assumed
> that the impact is proportional to the use of this feature, in which case
> reducing the amount of rewrite would help considerably.
>
> But as I look more into it, it feels less and less like that. The picture
> I'm getting now is that JENKINS-28799 is about BCT not updating
> StackMapFrame properly, and our attempt to fix it introduced other problems
> like JENKINS-31019.
>
> If those are the only issue, I still think the overall bytecode
> transformation strategy in BCT is sound. Given that stack map frame
> recomputation requires classloading, my suggestion is to tweak the
> transformation so that it doesn't require adjusting stack frame map. I think
> this is relatively easily achievable by moving the transformed code into a
> separate method so that bytecode index remain the same.
>
> That is, whereas today the transformation is as follows:
>
>     ... bunch of bytecode ...
>     GETFIELD com.example.Foo#id
>     ... bunch of bytecode ...
>
>           =>
>
>     ... bunch of bytecode ...
>     if ( LHS instanceof TYPE ) {
>          INVOKEVIRTUAL com.example.Foo#getId()
>     } else {
>          GETFIELD com.example.Foo#id
>     }
>     ... bunch of bytecode ...
>
> Replace this with:
>
>     ... bunch of bytecode ...
>     INVOKSTATIC helperMethodWeAdd(Foo)
>     ... bunch of bytecode ...
>
> ... where the helper method is the if/else block. The stack map analysis of
> the helper method does not contain any joinining of flow, so there's no need
> to compute the common base type, and hence it shouldn't require any
> classloading.
>
> (Alternatively, I think it is also possible to just update existing stack
> frame map incrementally by inserting additional intermediate frames.)
>
> --
> Kohsuke Kawaguchi
>
> --
> You received this message because you are subscribed to the Google Groups
> "Jenkins Developers" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to [hidden email].
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/jenkinsci-dev/CAN4CQ4zidKNqzy-7HJhbEEj2A_jU1zK5k75hapCG-QDScQhHzA%40mail.gmail.com.
>
> For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CA%2BnPnMzs%2BRWcUnR1mEX_MoAe9hQcwkkZmn055wb3bZkgeWQn8w%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: 2.0 proposal

Jesse Glick-4
In reply to this post by Kohsuke Kawaguchi
On Tue, Oct 20, 2015 at 2:38 PM, Kohsuke Kawaguchi <[hidden email]> wrote:
> …this is relatively easily achievable by moving the transformed code into a
> separate method so that bytecode index remain the same.

I am not going to sleep more soundly tonight. :-)

I think the broader issue is that use of runtime bytecode manipulation
has proven to be subject to catastrophic bugs which are tricky to
understand, much less fix. And pity the developer who attaches a Java
debugger with a `*-sources.jar` and tries to step through rewritten
code. We should be considering simpler, lower-tech options. If a field
has been replaced with a getter, or whatever, let us make sure we fix
the deprecated source code references and release those fixes and make
sure users are not running the wrong version.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr1jDBcFNEAwXz2ja5YoMhYbC_JUAcDKnzY40g9pWQEXTw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: 2.0 proposal

slide
+1

On Tue, Oct 20, 2015 at 2:40 PM Jesse Glick <[hidden email]> wrote:
On Tue, Oct 20, 2015 at 2:38 PM, Kohsuke Kawaguchi <[hidden email]> wrote:
> …this is relatively easily achievable by moving the transformed code into a
> separate method so that bytecode index remain the same.

I am not going to sleep more soundly tonight. :-)

I think the broader issue is that use of runtime bytecode manipulation
has proven to be subject to catastrophic bugs which are tricky to
understand, much less fix. And pity the developer who attaches a Java
debugger with a `*-sources.jar` and tries to step through rewritten
code. We should be considering simpler, lower-tech options. If a field
has been replaced with a getter, or whatever, let us make sure we fix
the deprecated source code references and release those fixes and make
sure users are not running the wrong version.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr1jDBcFNEAwXz2ja5YoMhYbC_JUAcDKnzY40g9pWQEXTw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAPiUgVcT7gaiJwRXB2DsKSiG4%3D%3DMn4ChNWAVCGZkUTxxuDiGyw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

RE: 2.0 proposal

Artur Szostak
Guys, are you writing compilers / compiler tools or a continuous integration / deployment tool?
Fix the API's, refactor the source code, update the plugin code and recompile. Hop to it. ;-)
Don't use these kinds of hacks. It leads to the dark side.

Sorry if I sound harsh. But the things you are trying to do on a supposedly production ready system are insane. You are all making me more and more scared of upgrading or doing anything with my Jenkins system by the minute. :-(

Artur

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/3C350C9D1C9DEE47BAEA43102B433C9EBE5E1151%40HQMBX2.ads.eso.org.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: 2.0 proposal

Manuel Jesús Recena Soto-2
I totally agree!

2015-10-21 11:18 GMT+02:00 Artur Szostak <[hidden email]>:

> Guys, are you writing compilers / compiler tools or a continuous integration
> / deployment tool?
> Fix the API's, refactor the source code, update the plugin code and
> recompile. Hop to it. ;-)
> Don't use these kinds of hacks. It leads to the dark side.
>
> Sorry if I sound harsh. But the things you are trying to do on a supposedly
> production ready system are insane. You are all making me more and more
> scared of upgrading or doing anything with my Jenkins system by the minute.
> :-(
>
> Artur
>
> --
> You received this message because you are subscribed to the Google Groups
> "Jenkins Developers" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to [hidden email].
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/jenkinsci-dev/3C350C9D1C9DEE47BAEA43102B433C9EBE5E1151%40HQMBX2.ads.eso.org.
>
> For more options, visit https://groups.google.com/d/optout.



--
Manuel Recena Soto
* manuelrecena.com [/blog]
* linkedin.com/in/recena

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CABa-UofeYEYYQZqH9k8CkTqkQWWDE0x469By1yDn%3DO6YVmj06A%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: 2.0 proposal

Kanstantsin Shautsou-2
In reply to this post by Artur Szostak
That's what all people asking about last years, but there is number of old developers (not jenkins admins/users) that likes developing tricky code as it their main work.
KISS
On Wednesday, October 21, 2015 at 12:19:00 PM UTC+3, Artur Szostak wrote:
Guys, are you writing compilers / compiler tools or a continuous integration / deployment tool?
Fix the API's, refactor the source code, update the plugin code and recompile. Hop to it. ;-)
Don't use these kinds of hacks. It leads to the dark side.

Sorry if I sound harsh. But the things you are trying to do on a supposedly production ready system are insane. You are all making me more and more scared of upgrading or doing anything with my Jenkins system by the minute. :-(

Artur

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/f7d45f90-acfa-4422-b584-367dc66c56c3%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: 2.0 proposal

Kohsuke Kawaguchi
Administrator
In reply to this post by Jesse Glick-4

I feel like there's a gap in perception here. I'm sensing that you are feeling that this is a systematic problem where fixing one problem at hand wouldn't solve problems down the road. That there will be more of these problems down the road.

In contrast, what I see is one bug (failing to update StackMapFrame) and two failed attempts to fix it, and we are this close to fixing it properly. And class file format change that drives issues like this only happens at major Java releases, and Java9 has no such changes currently in it. Hopefully this helps you see why I feel like we are throwing the baby out with the bathwater.

And it's not that problems are hard to understand or hard to fix. What makes this issue painful is that it blows up in unexpected places.

Anyway, I'm losing this argument.

Now, if you are going to remove BCT altogether, what do we do with existing uses of them? Queue.Item#id is a relatively recent change. Are we just going to break plugins that use them?


2015-10-20 14:40 GMT-07:00 Jesse Glick <[hidden email]>:
On Tue, Oct 20, 2015 at 2:38 PM, Kohsuke Kawaguchi <[hidden email]> wrote:
> …this is relatively easily achievable by moving the transformed code into a
> separate method so that bytecode index remain the same.

I am not going to sleep more soundly tonight. :-)

I think the broader issue is that use of runtime bytecode manipulation
has proven to be subject to catastrophic bugs which are tricky to
understand, much less fix. And pity the developer who attaches a Java
debugger with a `*-sources.jar` and tries to step through rewritten
code. We should be considering simpler, lower-tech options. If a field
has been replaced with a getter, or whatever, let us make sure we fix
the deprecated source code references and release those fixes and make
sure users are not running the wrong version.


--
Kohsuke Kawaguchi

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAN4CQ4xvMGgXCdVPttYu-_kD7YLmJqzkrn87gWY2K457pF87mQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: 2.0 proposal

Jesse Glick-4
On Wed, Oct 21, 2015 at 1:28 PM, Kohsuke Kawaguchi <[hidden email]> wrote:
> That there will be more of these problems down the road.

Perhaps. More important is the ongoing burden of not knowing whether
what you wrote in `src/main/java/**/*.java`, or what you loaded from
`*-sources.jar`, is actually what is running.

> I feel like we are throwing the baby out with the bathwater.

This baby is Otesánek. :-)

> it's not that problems are hard to understand or hard to fix.

For you perhaps.

> if you are going to remove BCT altogether, what do we do with existing
> uses of them? Queue.Item#id is a relatively recent change. Are we just going
> to break plugins that use them?

There was no fully developed proposal yet. My idea was to have an
automated system for detecting plugin source repos still using the
deprecated API, then fixing those, and concurrently blocking the old
plugin releases from being used. So a user would never see linkage
errors, but they might be required to upgrade a plugin when upgrading
core (or a lower-level plugin).

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr3MSQ4pX14%3DdepUc%3D28WOV6Ny2RNyaCgN4Gkd65z1esVA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.