Re: RFR: 8308899: Introduce Classfile.Context and improve Classfile.Option(s) [v8]

2023-06-08 Thread Adam Sotona 
On Thu, 8 Jun 2023 11:07:21 GMT, Chen Liang  wrote:

>> Adam Sotona has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 29 commits:
>> 
>>  - removal of ClassHierarchyImpl.DEFAULT_RESOLVER
>>introduction of ClassHierarchyResolver::ofSystem factory method
>>ClassfileImpl does not pre-initialize ClassHierarchyResolverOption with 
>> default
>>  - Merge branch 'master' into JDK-8308899-context
>>
>># Conflicts:
>># src/java.base/share/classes/jdk/internal/classfile/impl/Options.java
>># 
>> src/java.base/share/classes/jdk/internal/classfile/snippet-files/PackageSnippets.java
>># test/jdk/jdk/classfile/ClassHierarchyInfoTest.java
>># test/jdk/jdk/classfile/VerifierSelfTest.java
>>  - Merge branch 'master' into JDK-8308899-context
>>
>># Conflicts:
>># src/java.base/share/classes/jdk/internal/classfile/Classfile.java
>># 
>> src/java.base/share/classes/jdk/internal/classfile/impl/DirectCodeBuilder.java
>># test/jdk/jdk/classfile/DiscontinuedInstructionsTest.java
>># test/jdk/jdk/classfile/FilterDeadLabelsTest.java
>># test/jdk/jdk/classfile/ShortJumpsFixTest.java
>># test/jdk/jdk/classfile/StackMapsTest.java
>>  - added missing javadoc
>>  - simplified options names
>>  - fixed copyright header
>>  - Merge branch 'master' into JDK-8308899-context
>>  - fixed StackMapGenerator::generatorError and removed obsolete 
>> SplitConstantPool clone constructor
>>  - Merge branch 'master' into JDK-8308899-context
>>  - fixed benchmarks
>>  - ... and 19 more: https://git.openjdk.org/jdk/compare/ac3ce2bf...aa691842
>
> src/java.base/share/classes/jdk/internal/classfile/ClassHierarchyResolver.java
>  line 66:
> 
>> 64: @Override
>> 65: public Map> ClassHierarchyResolver.ClassHierarchyInfo> get() {
>> 66: return new ConcurrentHashMap<>();
> 
> Don't think we need to synchronize the cache if we have dedicated CHRs.

Fixed, thanks.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14180#discussion_r1222902932

Re: RFR: 8308899: Introduce Classfile.Context and improve Classfile.Option(s) [v8]

2023-06-08 Thread Chen Liang 
On Thu, 8 Jun 2023 09:26:10 GMT, Adam Sotona  wrote:

>> Classfile context object and multi-state options have been discussed at 
>> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-May/000321.html
>> This patch implements the proposed changes in Classfile API and fixes all 
>> affected code across JDK sources and tests.
>> 
>> Please review.
>> 
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 29 commits:
> 
>  - removal of ClassHierarchyImpl.DEFAULT_RESOLVER
>introduction of ClassHierarchyResolver::ofSystem factory method
>ClassfileImpl does not pre-initialize ClassHierarchyResolverOption with 
> default
>  - Merge branch 'master' into JDK-8308899-context
>
># Conflicts:
>#  src/java.base/share/classes/jdk/internal/classfile/impl/Options.java
>#  
> src/java.base/share/classes/jdk/internal/classfile/snippet-files/PackageSnippets.java
>#  test/jdk/jdk/classfile/ClassHierarchyInfoTest.java
>#  test/jdk/jdk/classfile/VerifierSelfTest.java
>  - Merge branch 'master' into JDK-8308899-context
>
># Conflicts:
>#  src/java.base/share/classes/jdk/internal/classfile/Classfile.java
>#  
> src/java.base/share/classes/jdk/internal/classfile/impl/DirectCodeBuilder.java
>#  test/jdk/jdk/classfile/DiscontinuedInstructionsTest.java
>#  test/jdk/jdk/classfile/FilterDeadLabelsTest.java
>#  test/jdk/jdk/classfile/ShortJumpsFixTest.java
>#  test/jdk/jdk/classfile/StackMapsTest.java
>  - added missing javadoc
>  - simplified options names
>  - fixed copyright header
>  - Merge branch 'master' into JDK-8308899-context
>  - fixed StackMapGenerator::generatorError and removed obsolete 
> SplitConstantPool clone constructor
>  - Merge branch 'master' into JDK-8308899-context
>  - fixed benchmarks
>  - ... and 19 more: https://git.openjdk.org/jdk/compare/ac3ce2bf...aa691842

src/java.base/share/classes/jdk/internal/classfile/ClassHierarchyResolver.java 
line 66:

> 64: @Override
> 65: public Map ClassHierarchyResolver.ClassHierarchyInfo> get() {
> 66: return new ConcurrentHashMap<>();

Don't think we need to synchronize the cache if we have dedicated CHRs.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14180#discussion_r1222886769

Re: RFR: 8308899: Introduce Classfile.Context and improve Classfile.Option(s) [v8]

2023-06-08 Thread Brian Goetz 
On Thu, 8 Jun 2023 09:26:10 GMT, Adam Sotona  wrote:

>> Classfile context object and multi-state options have been discussed at 
>> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-May/000321.html
>> This patch implements the proposed changes in Classfile API and fixes all 
>> affected code across JDK sources and tests.
>> 
>> Please review.
>> 
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 29 commits:
> 
>  - removal of ClassHierarchyImpl.DEFAULT_RESOLVER
>introduction of ClassHierarchyResolver::ofSystem factory method
>ClassfileImpl does not pre-initialize ClassHierarchyResolverOption with 
> default
>  - Merge branch 'master' into JDK-8308899-context
>
># Conflicts:
>#  src/java.base/share/classes/jdk/internal/classfile/impl/Options.java
>#  
> src/java.base/share/classes/jdk/internal/classfile/snippet-files/PackageSnippets.java
>#  test/jdk/jdk/classfile/ClassHierarchyInfoTest.java
>#  test/jdk/jdk/classfile/VerifierSelfTest.java
>  - Merge branch 'master' into JDK-8308899-context
>
># Conflicts:
>#  src/java.base/share/classes/jdk/internal/classfile/Classfile.java
>#  
> src/java.base/share/classes/jdk/internal/classfile/impl/DirectCodeBuilder.java
>#  test/jdk/jdk/classfile/DiscontinuedInstructionsTest.java
>#  test/jdk/jdk/classfile/FilterDeadLabelsTest.java
>#  test/jdk/jdk/classfile/ShortJumpsFixTest.java
>#  test/jdk/jdk/classfile/StackMapsTest.java
>  - added missing javadoc
>  - simplified options names
>  - fixed copyright header
>  - Merge branch 'master' into JDK-8308899-context
>  - fixed StackMapGenerator::generatorError and removed obsolete 
> SplitConstantPool clone constructor
>  - Merge branch 'master' into JDK-8308899-context
>  - fixed benchmarks
>  - ... and 19 more: https://git.openjdk.org/jdk/compare/ac3ce2bf...aa691842

Agreed.  The need for sync came from the shared static cache.

On 6/8/2023 7:07 AM, liach wrote:
>
> ***@***. commented on this pull request.
>
> 
>
> In 
> src/java.base/share/classes/jdk/internal/classfile/ClassHierarchyResolver.java
>  
> :
>
> >   */
> -static ClassHierarchyResolver defaultResolver() {
> -return ClassHierarchyImpl.DEFAULT_RESOLVER;
> +static ClassHierarchyResolver ofSystem() {
> +var sysLoader = ClassLoader.getSystemClassLoader();
> +return ClassHierarchyResolver
> +.ofResourceParsing(sysLoader)
> +.orElse(ClassHierarchyResolver.ofClassLoading(sysLoader))
> +.cached(new Supplier<>() {
> +@Override
> +public Map ClassHierarchyResolver.ClassHierarchyInfo> get() {
> +return new ConcurrentHashMap<>();
>
> Don't think we need to synchronize the cache if we have dedicated CHRs.
>
> —
> Reply to this email directly, view it on GitHub 
> , 
> or unsubscribe 
> .
> You are receiving this because you commented.Message ID: 
> ***@***.***>
>

-

PR Comment: https://git.openjdk.org/jdk/pull/14180#issuecomment-1582385327

Re: RFR: 8308899: Introduce Classfile.Context and improve Classfile.Option(s) [v8]

2023-06-08 Thread Adam Sotona 
> Classfile context object and multi-state options have been discussed at 
> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-May/000321.html
> This patch implements the proposed changes in Classfile API and fixes all 
> affected code across JDK sources and tests.
> 
> Please review.
> 
> Thanks,
> Adam

Adam Sotona has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains 29 commits:

 - removal of ClassHierarchyImpl.DEFAULT_RESOLVER
   introduction of ClassHierarchyResolver::ofSystem factory method
   ClassfileImpl does not pre-initialize ClassHierarchyResolverOption with 
default
 - Merge branch 'master' into JDK-8308899-context
   
   # Conflicts:
   #src/java.base/share/classes/jdk/internal/classfile/impl/Options.java
   #
src/java.base/share/classes/jdk/internal/classfile/snippet-files/PackageSnippets.java
   #test/jdk/jdk/classfile/ClassHierarchyInfoTest.java
   #test/jdk/jdk/classfile/VerifierSelfTest.java
 - Merge branch 'master' into JDK-8308899-context
   
   # Conflicts:
   #src/java.base/share/classes/jdk/internal/classfile/Classfile.java
   #
src/java.base/share/classes/jdk/internal/classfile/impl/DirectCodeBuilder.java
   #test/jdk/jdk/classfile/DiscontinuedInstructionsTest.java
   #test/jdk/jdk/classfile/FilterDeadLabelsTest.java
   #test/jdk/jdk/classfile/ShortJumpsFixTest.java
   #test/jdk/jdk/classfile/StackMapsTest.java
 - added missing javadoc
 - simplified options names
 - fixed copyright header
 - Merge branch 'master' into JDK-8308899-context
 - fixed StackMapGenerator::generatorError and removed obsolete 
SplitConstantPool clone constructor
 - Merge branch 'master' into JDK-8308899-context
 - fixed benchmarks
 - ... and 19 more: https://git.openjdk.org/jdk/compare/ac3ce2bf...aa691842

-

Changes: https://git.openjdk.org/jdk/pull/14180/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14180&range=07
  Stats: 1651 lines in 109 files changed: 501 ins; 244 del; 906 mod
  Patch: https://git.openjdk.org/jdk/pull/14180.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14180/head:pull/14180

PR: https://git.openjdk.org/jdk/pull/14180