2015-10-20 2:53 GMT-07:00 James Nord <jn...@cloudbees.com>: > > > 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 <jgl...@cloudbees.com>: >> >>> On Mon, Oct 19, 2015 at 2:19 PM, Kohsuke Kawaguchi <k...@kohsuke.org> >>> 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 <https://issues.jenkins-ci.org/browse/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 jenkinsci-dev+unsubscr...@googlegroups.com. 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.
