[External] : AW: Remove stream usage from Symtab.lookupPackage()

[Vicente Romero]

Hi Christoph,

I'm on the fence with this one. The improvements doesn't seems to be 
considerable, while some could see the rewriting as step back given 
that, again could be subjective, the code could be seen as more readable 
using Streams. We have seen great performance improvements overtime in 
new features like lambdas etc. I can certainly expect this to happen to 
streams too. So I don't see the same benefit here as in your other PR, 
the Pretty.java one. But others in the list could have another opinion,

Thanks,
Vicente

On 2/22/23 06:46, christoph.dreis at freenet.de wrote:
>
> Hi Vicente,
>
> thanks for taking the time. I’ve added a comment in the PR with 
> screenshots from async-profiler runs etc. that are somewhat easier to 
> post there compared to the mailing list. But to share the TL;DR:
>
>   * ~2500 classes
>   * Average size: ~76 lines
>   * Largest class: ~2500 lines
>   * Mostly hand-written, but some auto-generated protobuf files (I'd
>     say 90/10 split)
>
> Overall timings didn’t improve (or regress), but the allocations of 
> Symtab.lookupPackage() went down from 1.6% to 0.1% overall, while CPU 
> frames went down from 1.6% to 0.9%.
>
> Let me know if you need more information and I’ll post this to the 
> issue on GitHub.
>
> Cheers,
>
> Christoph
>
> Am 22.02.23, 04:16 schrieb "Vicente Romero" <vicente.romero at oracle.com>:
>
>
> Hi Christoph,
>
> Could you please share more info like what type of sources are you 
> compiling: machine generated or not, very long sources, etc. Also 
> what's the performance improvement you get with your patch, etc.
>
> Thanks,
> Vicente
>
> On 2/21/23 16:03, christoph.dreis at freenet.de wrote:
>
>     Hi,
>
>     I'm currently profiling some compilation phases of internal
>     projects and noticed in allocation profiles that
>     |Symtab.lookupPackage| takes up ~2% overall. The majority of this
>     is spent in |.stream().anyMatch()| usages to find out if the given
>     module symbol depends on the unnamed module.
>
>     The PR under https://github.com/openjdk/jdk/pull/12700
>     <https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/12700__;!!ACWV5N9M2RV99hQ!IIXyUx5ZjLrx7hFkIokefPwtWpLqF1uyRI7MXKKY1m72UryCkRemnzgHLC2txnHo3SZFHozLPgWBvjDFql3VAAz88x1M6lU4$>
>     desugars the code into a simple loop.
>
>     If you think this is worthwhile I'd appreciate if this is
>     sponsored. I'd also need a ticket number for that because I can't
>     create tickets. CLA should be signed though.
>
>     Let me know what you think.
>     Cheers,
>     Christoph
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/compiler-dev/attachments/20230222/b76b91b3/attachment-0001.htm>