rmannibucau commented on pull request #95: URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-846870345
Hi @kriegaex , as said last week I had a look to this and have multiple comments to do: 1. I tested the 1 remapper instance per visitor (ClassRemapper) and perfs are not impacted it seems (as expected since the visitor will allocate already way more instance than the class visitor) so I think it can be a good simplicity/perf/design solution actually 2. I tested an alternative to define a custom reduced remapper API to reduce the API surface to what we use (https://github.com/rmannibucau/maven-shade-plugin/commit/50e101feb19f4b82d14346afe613fe6064db99c4), it still relies on "1" (or its variant to set the visitor in the remapper to avoid this allocation but don't think it is worth) but makes the relocating API we use not dependent on ASM which simplifies it a bit at the cost of another interface. Think "1" is simpler but this option has a nice hidden gem: it enables to reimplement ClassRemapper which has limitations in relocations and is inconsistent with our configuration (we dont relocate bytecode but packages only semantically - not sure it is intended but what all the impl do). Don't think we need to go that far for now - once again I'm actually pretty happy with 1 since it is fast and does the work. So overall, after some more advanced testing on small and "medium/big" (~50M) fatjars with relocations, I think it is good to just store the remapped flag in the remapper and have one instance per class jar entry. It avoids some verbosity we can try to avoid until we do a complete and adapted implementation (which would be way more than this PR tries to do IMHO). Wdyt? -- 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. For queries about this service, please contact Infrastructure at: [email protected]
