You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@aries.apache.org by GitBox <gi...@apache.org> on 2022/12/12 22:01:00 UTC
[GitHub] [aries] stiemannkj1 opened a new pull request, #215: ARIES-1219 Weaving of Aries SPI Fly bundle produces RuntimeException: SR/RET are not supported with computeFrames option
stiemannkj1 opened a new pull request, #215:
URL: https://github.com/apache/aries/pull/215
Fixes https://issues.apache.org/jira/browse/ARIES-1219
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@aries.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [aries] vbansal22 commented on pull request #215: ARIES-1219 Weaving of Aries SPI Fly bundle produces RuntimeException: JSR/RET are not supported with computeFrames option
Posted by "vbansal22 (via GitHub)" <gi...@apache.org>.
vbansal22 commented on PR #215:
URL: https://github.com/apache/aries/pull/215#issuecomment-1494961160
Also tested the fix. Thanks @stiemannkj1
It works well. Please merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@aries.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [aries] vbansal22 commented on pull request #215: ARIES-1219 Weaving of Aries SPI Fly bundle produces RuntimeException: JSR/RET are not supported with computeFrames option
Posted by "vbansal22 (via GitHub)" <gi...@apache.org>.
vbansal22 commented on PR #215:
URL: https://github.com/apache/aries/pull/215#issuecomment-1485997835
Can we merge this please? I have run into to a similar issue. Would like to see this fixed. I have reviewed the code and it looks good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@aries.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [aries] stiemannkj1 commented on pull request #215: ARIES-1219 Weaving of Aries SPI Fly bundle produces RuntimeException: JSR/RET are not supported with computeFrames option
Posted by "stiemannkj1 (via GitHub)" <gi...@apache.org>.
stiemannkj1 commented on PR #215:
URL: https://github.com/apache/aries/pull/215#issuecomment-1495042270
> I’m not sure why the Java 8 check is present? Is there a reason not to include JSR inlining at all times?
Only performance. I can add a comment to that effect but Java 8+ compiled classes are guaranteed not have JSR/RET instructions. I believe weaving takes place for every class loaded after SPI-Fly is activated, so it could be quite costly to apply these unnecessary checks when the vast majority of classes are Java 8+ these days. I'd like to keep the check along with a comment as I don't think it adds significant complexity.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@aries.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [aries] timothyjward merged pull request #215: ARIES-1219 Weaving of Aries SPI Fly bundle produces RuntimeException: JSR/RET are not supported with computeFrames option
Posted by "timothyjward (via GitHub)" <gi...@apache.org>.
timothyjward merged PR #215:
URL: https://github.com/apache/aries/pull/215
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@aries.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [aries] stiemannkj1 commented on pull request #215: ARIES-1219 Weaving of Aries SPI Fly bundle produces RuntimeException: JSR/RET are not supported with computeFrames option
Posted by "stiemannkj1 (via GitHub)" <gi...@apache.org>.
stiemannkj1 commented on PR #215:
URL: https://github.com/apache/aries/pull/215#issuecomment-1494966736
@vbansal22 unfortunately, I don't have the ability to merge this. Still awaiting review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@aries.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [aries] timothyjward commented on pull request #215: ARIES-1219 Weaving of Aries SPI Fly bundle produces RuntimeException: JSR/RET are not supported with computeFrames option
Posted by "timothyjward (via GitHub)" <gi...@apache.org>.
timothyjward commented on PR #215:
URL: https://github.com/apache/aries/pull/215#issuecomment-1495528002
> I appreciate that my case is not typical. But if it can be supported with little to no penalty, that would be great.
@wilx I don't think that there is any need to drop support for older bytecode versions, and there's nothing in this PR that would do so.
> Only performance. I can add a comment to that effect but Java 8+ compiled classes are guaranteed not have JSR/RET instructions.
@stiemannkj1 - that's what I thought. Can we remove those parts of this PR please? It's not good practice to mix bug fixes with feature enhancements, and it will confuse any regression analysis later if there are problems. I believe that this is also the wrong point in the code to optimise performance.
> I believe weaving takes place for every class loaded after SPI-Fly is activated, so it could be quite costly to apply these unnecessary checks when the vast majority of classes are Java 8+ these days.
SPI-Fly already ignores bundles that haven't opted in to weaving, which is most of them, so this isn't as big a deal as you might think. If we were looking to improve performance further then we would:
1. Need to do some measurements. If we don't do this then we have no idea if it's worth it.
2. Look at higher-level options for skipping work, e.g. a two-stage check/weave model.
The current implementation writes a woven class file every time, but throws it away if there were no calls to decorate. Given that most class files don't need decoration (even in an opted-in bundle) it would be much faster to run a "check" which ignores everything but the method bodies to determine whether decoration is needed. Only if decoration is needed do we then run the full weaving code and generate the updated class bytes. The checker could also identify:
* Whether any methods contain JSR instructions so that decorator is only added if needed.
* Which methods need decoration so that we can avoid unnecessary re-checking in the weaving code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@aries.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [aries] raghavendrav commented on pull request #215: ARIES-1219 Weaving of Aries SPI Fly bundle produces RuntimeException: JSR/RET are not supported with computeFrames option
Posted by "raghavendrav (via GitHub)" <gi...@apache.org>.
raghavendrav commented on PR #215:
URL: https://github.com/apache/aries/pull/215#issuecomment-1496471972
Hi @stiemannkj1 ,
The below file is missing a header and is getting flagged when doing a mvn clean install on spi-fly.
I worked around by adding the header myself locally.
[TCCLSetterVisitorTest.java]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@aries.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [aries] bosschaert commented on pull request #215: ARIES-1219 Weaving of Aries SPI Fly bundle produces RuntimeException: JSR/RET are not supported with computeFrames option
Posted by "bosschaert (via GitHub)" <gi...@apache.org>.
bosschaert commented on PR #215:
URL: https://github.com/apache/aries/pull/215#issuecomment-1486320133
@timothyjward would you have time to review this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@aries.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [aries] vbansal22 commented on pull request #215: ARIES-1219 Weaving of Aries SPI Fly bundle produces RuntimeException: JSR/RET are not supported with computeFrames option
Posted by "vbansal22 (via GitHub)" <gi...@apache.org>.
vbansal22 commented on PR #215:
URL: https://github.com/apache/aries/pull/215#issuecomment-1532335863
Hi @timothyjward , is it possible to trigger a release build (1.3.7) after the merge?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@aries.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [aries] timothyjward commented on pull request #215: ARIES-1219 Weaving of Aries SPI Fly bundle produces RuntimeException: JSR/RET are not supported with computeFrames option
Posted by "timothyjward (via GitHub)" <gi...@apache.org>.
timothyjward commented on PR #215:
URL: https://github.com/apache/aries/pull/215#issuecomment-1495018613
> @timothyjward would you have time to review this?
This code seems overly complex… I’m not sure why the Java 8 check is present? Is there a reason not to include JSR inlining at all times?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@aries.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [aries] stiemannkj1 commented on a diff in pull request #215: ARIES-1219 Weaving of Aries SPI Fly bundle produces RuntimeException: JSR/RET are not supported with computeFrames option
Posted by "stiemannkj1 (via GitHub)" <gi...@apache.org>.
stiemannkj1 commented on code in PR #215:
URL: https://github.com/apache/aries/pull/215#discussion_r1156490026
##########
spi-fly/spi-fly-itests/test.bndrun:
##########
@@ -51,16 +51,17 @@
bnd.identity;id='junit-platform-launcher'
-runbundles: \
- assertj-core;version='[3.22.0,3.22.1)',\
+ assertj-core;version='[3.23.1,3.23.2)',\
junit-jupiter-api;version='[5.8.2,5.8.3)',\
junit-jupiter-engine;version='[5.8.2,5.8.3)',\
junit-jupiter-params;version='[5.8.2,5.8.3)',\
junit-platform-commons;version='[1.8.2,1.8.3)',\
junit-platform-engine;version='[1.8.2,1.8.3)',\
junit-platform-launcher;version='[1.8.2,1.8.3)',\
- org.apache.aries.spifly.dynamic.framework.extension;version='[1.3.5,1.3.6)',\
- org.apache.aries.spifly.examples.spi.bundle;version='[1.0.3,1.0.4)',\
- org.apache.aries.spifly.itests;version='[1.3.5,1.3.6)',\
+ net.bytebuddy.byte-buddy;version='[1.12.10,1.12.11)',\
+ org.apache.aries.spifly.dynamic.framework.extension;version='[1.4.0,1.4.1)',\
+ org.apache.aries.spifly.examples.spi.bundle;version='[1.0.5,1.0.6)',\
+ org.apache.aries.spifly.itests;version='[1.4.0,1.4.1)',\
Review Comment:
These changes took place automatically when I performed a local build, so I think they might have been missed in a previous update.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@aries.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [aries] bosschaert commented on pull request #215: ARIES-1219 Weaving of Aries SPI Fly bundle produces RuntimeException: JSR/RET are not supported with computeFrames option
Posted by GitBox <gi...@apache.org>.
bosschaert commented on PR #215:
URL: https://github.com/apache/aries/pull/215#issuecomment-1347918692
It's been a while since I looked at this code, but on the surface the change looks good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@aries.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [aries] stiemannkj1 commented on pull request #215: ARIES-1219 Weaving of Aries SPI Fly bundle produces RuntimeException: JSR/RET are not supported with computeFrames option
Posted by "stiemannkj1 (via GitHub)" <gi...@apache.org>.
stiemannkj1 commented on PR #215:
URL: https://github.com/apache/aries/pull/215#issuecomment-1500880937
@timothyjward can you merge when you get the chance? I don’t have permission. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@aries.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [aries] vbansal22 commented on pull request #215: ARIES-1219 Weaving of Aries SPI Fly bundle produces RuntimeException: JSR/RET are not supported with computeFrames option
Posted by "vbansal22 (via GitHub)" <gi...@apache.org>.
vbansal22 commented on PR #215:
URL: https://github.com/apache/aries/pull/215#issuecomment-1502673690
Thanks @stiemannkj1 and @timothyjward
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@aries.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [aries] wilx commented on pull request #215: ARIES-1219 Weaving of Aries SPI Fly bundle produces RuntimeException: JSR/RET are not supported with computeFrames option
Posted by "wilx (via GitHub)" <gi...@apache.org>.
wilx commented on PR #215:
URL: https://github.com/apache/aries/pull/215#issuecomment-1495427913
Not everyone using this is running Java 8+ code. I have some legacy apps that use OSGi that use SPI Fly where I had this issue because they are using components compiled as Java 5 (or older). They are using [Doug Lea's concurrency](http://gee.cs.oswego.edu/dl/). Rewriting the code to use modern concurrency classes is not an option, unfortunately. Beside some Java 5 (or older), many of the dependencies are Java 6 or 7, too.
I appreciate that my case is not typical. But if it can be supported with little to no penalty, that would be great.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@aries.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [aries] stiemannkj1 commented on pull request #215: ARIES-1219 Weaving of Aries SPI Fly bundle produces RuntimeException: JSR/RET are not supported with computeFrames option
Posted by "stiemannkj1 (via GitHub)" <gi...@apache.org>.
stiemannkj1 commented on PR #215:
URL: https://github.com/apache/aries/pull/215#issuecomment-1500619908
@timothyjward, simplified to add the bare minimum of `JSRInlinerAdapter`. Should be good to merge now unless you notice any other issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@aries.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org