Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v5]
On Mon, 3 Mar 2025 20:02:16 GMT, Ivan Walulya wrote: >> Thomas Schatzl has updated the pull request incrementally with one >> additional commit since the last revision: >> >> * fix comment (trailing whitespace) >> * another assert when snapshotting at a safepoint. > > src/hotspot/share/gc/g1/g1ConcurrentRefineStats.hpp line 43: > >> 41: size_t _cards_clean;// Number of cards found clean. >> 42: size_t _cards_not_parsable; // Number of cards we could not >> parse and left unrefined. >> 43: size_t _cards_still_refer_to_cset; // Number of cards marked still >> young. > > `_cards_still_refer_to_cset` from the naming it is not clear what the > difference is with `_cards_refer_to_cset`, the comment is not helping with > that `cards_still_refer_to_cset` refers to cards that were found to have already been marked as `to-collection-set`. Renamed to `_cards_already_refer_to_cset`, would that be okay? - PR Review Comment: https://git.openjdk.org/jdk/pull/23739#discussion_r1978868225
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v5]
On Mon, 3 Mar 2025 18:28:48 GMT, Ivan Walulya wrote: > Why are we using a prediction here? Quickly checking again, do we have the actual count here from somewhere? > Additionally, won't this prediction also include cards from the old gen > regions in case of mixed gcs? How do we reconcile that when we are adding old > gen regions to c-set? The predictor contents changed to (supposedly) only contain cards containing young gen references. See g1Policy.cpp:934: _analytics->report_card_rs_length(total_cards_scanned - total_non_young_rs_cards, is_young_only_pause); - PR Review Comment: https://git.openjdk.org/jdk/pull/23739#discussion_r1978876199
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v5]
On Mon, 3 Mar 2025 15:19:20 GMT, Albert Mingkun Yang wrote: > Can you elaborate on what the "special handling" would be, if we don's set > "claimed" for non-committed regions? the iteration code, would for every region check whether the region is actually committed or not. The `heap_region_iterate()` API of `G1CollectedHeap` only iterates over committed regions. So only committed regions will be updated in the state table. Later when iterating over the state table, the code uses the array directly, i.e. the claim state of uncommitted regions would be read as uninitialized. Further, it would be hard to exclude regions committed after the snapshot otherwise (we do not need to iterate over them. Their card table can't contain card marks) as we do not track newly committed regions in the snapshot. We could do, but would be a headache due to memory synchronization because regions can be committed any time. Imho it is much simpler to reset all the card claims to "already processed" and then make the regions we want to work on claimable. - PR Review Comment: https://git.openjdk.org/jdk/pull/23739#discussion_r1978893134
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v9]
On Tue, 4 Mar 2025 10:06:37 GMT, Ivan Walulya wrote:
>> Thomas Schatzl has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> * iwalulya review 2
>> * G1ConcurrentRefineWorkState -> G1ConcurrentRefineSweepState
>> * some additional documentation
>
> src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp line 108:
>
>> 106:
>> 107: void G1ConcurrentRefineThreadControl::control_thread_do(ThreadClosure*
>> tc) {
>> 108: if (_control_thread != nullptr) {
>
> maybe maintain using `if (max_num_threads() > 0)` as used in
> `G1ConcurrentRefineThreadControl::initialize`, so that it is clear that
> setting `G1ConcRefinementThreads=0` effectively turns off concurrent
> refinement.
I added a new `is_refinement_enabled()` predicate instead (that uses
`max_num_threads()`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23739#discussion_r1979252156
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v11]
> Hi all, > > please review this change that implements (currently Draft) JEP: G1: > Improve Application Throughput with a More Efficient Write-Barrier. > > The reason for posting this early is that this is a large change, and the JEP > process is already taking very long with no end in sight but we would like to > have this ready by JDK 25. > > ### Current situation > > With this change, G1 will reduce the post write barrier to much more resemble > Parallel GC's as described in the JEP. The reason is that G1 lacks in > throughput compared to Parallel/Serial GC due to larger barrier. > > The main reason for the current barrier is how g1 implements concurrent > refinement: > * g1 tracks dirtied cards using sets (dirty card queue set - dcqs) of buffers > (dirty card queues - dcq) containing the location of dirtied cards. > Refinement threads pick up their contents to re-refine. The barrier needs to > enqueue card locations. > * For correctness dirty card updates requires fine-grained synchronization > between mutator and refinement threads, > * Finally there is generic code to avoid dirtying cards altogether (filters), > to avoid executing the synchronization and the enqueuing as much as possible. > > These tasks require the current barrier to look as follows for an assignment > `x.a = y` in pseudo code: > > > // Filtering > if (region(@x.a) == region(y)) goto done; // same region check > if (y == null) goto done; // null value check > if (card(@x.a) == young_card) goto done; // write to young gen check > StoreLoad;// synchronize > if (card(@x.a) == dirty_card) goto done; > > *card(@x.a) = dirty > > // Card tracking > enqueue(card-address(@x.a)) into thread-local-dcq; > if (thread-local-dcq is not full) goto done; > > call runtime to move thread-local-dcq into dcqs > > done: > > > Overall this post-write barrier alone is in the range of 40-50 total > instructions, compared to three or four(!) for parallel and serial gc. > > The large size of the inlined barrier not only has a large code footprint, > but also prevents some compiler optimizations like loop unrolling or inlining. > > There are several papers showing that this barrier alone can decrease > throughput by 10-20% > ([Yang12](https://dl.acm.org/doi/10.1145/2426642.2259004)), which is > corroborated by some benchmarks (see links). > > The main idea for this change is to not use fine-grained synchronization > between refinement and mutator threads, but coarse grained based on > atomically switching card tables. Mutators only work on the "primary" card > table, refinement threads on a se... Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: iwalulya review * comments for variables tracking to-collection-set and just dirtied cards after GC/refinement * predicate for determining whether the refinement has been disabled * some other typos/comment improvements * renamed _has_xxx_ref to _has_ref_to_xxx to be more consistent with naming - Changes: - all: https://git.openjdk.org/jdk/pull/23739/files - new: https://git.openjdk.org/jdk/pull/23739/files/fc674f02..b4d19d9b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23739&range=10 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23739&range=09-10 Stats: 40 lines in 8 files changed: 14 ins; 0 del; 26 mod Patch: https://git.openjdk.org/jdk/pull/23739.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23739/head:pull/23739 PR: https://git.openjdk.org/jdk/pull/23739
Re: RFR: 8350617: Improve MethodHandles.tableSwitch and remove intrinsicData
On Tue, 4 Mar 2025 14:26:19 GMT, Chen Liang wrote: >> src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java >> line 628: >> >>> 626: continue; >>> 627: } >>> 628: break; // Only inline target MHs >>> if this is customized >> >> I think this could be problematic, as we typically only customize the root >> method handle in a chain. So, if a table switch handle is used with another >> combinator, we will never benefit from the intrinsic. > > In that case, won't the root form be customized and the table switch names be > inlined into the root form? The root would e.g. have a constant BMH pointing at a _shared_ tableSwitch LF. So, the BMH fields would be seen as constant as well, including the list of cases. The issue is that the bytecode of the tableSwitch BMH will not be using the intrinsic, since it's not customized itself, i.e. it will only have a single call site for all cases. - PR Review Comment: https://git.openjdk.org/jdk/pull/23763#discussion_r1979585627
Re: RFR: 8350617: Improve MethodHandles.tableSwitch and remove intrinsicData
On Tue, 4 Mar 2025 14:17:59 GMT, Chen Liang wrote: > The LambdaForm will be only used for non-customized bytecode (which cannot > fully inline anyways) True, without customization the cases holder in the current implementation will not be a constant, so we can never inline the cases for shared LFs. However, I think if the root MH in a chain is customized, we would still get inlining, whereas with the new implementation we don't, as we skipped generating the intrinsic... - PR Comment: https://git.openjdk.org/jdk/pull/23763#issuecomment-2697820353
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v11]
On Tue, 4 Mar 2025 11:56:56 GMT, Thomas Schatzl wrote:
>> Hi all,
>>
>> please review this change that implements (currently Draft) JEP: G1:
>> Improve Application Throughput with a More Efficient Write-Barrier.
>>
>> The reason for posting this early is that this is a large change, and the
>> JEP process is already taking very long with no end in sight but we would
>> like to have this ready by JDK 25.
>>
>> ### Current situation
>>
>> With this change, G1 will reduce the post write barrier to much more
>> resemble Parallel GC's as described in the JEP. The reason is that G1 lacks
>> in throughput compared to Parallel/Serial GC due to larger barrier.
>>
>> The main reason for the current barrier is how g1 implements concurrent
>> refinement:
>> * g1 tracks dirtied cards using sets (dirty card queue set - dcqs) of
>> buffers (dirty card queues - dcq) containing the location of dirtied cards.
>> Refinement threads pick up their contents to re-refine. The barrier needs to
>> enqueue card locations.
>> * For correctness dirty card updates requires fine-grained synchronization
>> between mutator and refinement threads,
>> * Finally there is generic code to avoid dirtying cards altogether
>> (filters), to avoid executing the synchronization and the enqueuing as much
>> as possible.
>>
>> These tasks require the current barrier to look as follows for an assignment
>> `x.a = y` in pseudo code:
>>
>>
>> // Filtering
>> if (region(@x.a) == region(y)) goto done; // same region check
>> if (y == null) goto done; // null value check
>> if (card(@x.a) == young_card) goto done; // write to young gen check
>> StoreLoad;// synchronize
>> if (card(@x.a) == dirty_card) goto done;
>>
>> *card(@x.a) = dirty
>>
>> // Card tracking
>> enqueue(card-address(@x.a)) into thread-local-dcq;
>> if (thread-local-dcq is not full) goto done;
>>
>> call runtime to move thread-local-dcq into dcqs
>>
>> done:
>>
>>
>> Overall this post-write barrier alone is in the range of 40-50 total
>> instructions, compared to three or four(!) for parallel and serial gc.
>>
>> The large size of the inlined barrier not only has a large code footprint,
>> but also prevents some compiler optimizations like loop unrolling or
>> inlining.
>>
>> There are several papers showing that this barrier alone can decrease
>> throughput by 10-20%
>> ([Yang12](https://dl.acm.org/doi/10.1145/2426642.2259004)), which is
>> corroborated by some benchmarks (see links).
>>
>> The main idea for this change is to not use fine-grained synchronization
>> between refinement and mutator threads, but coarse grained based on
>> atomically switching c...
>
> Thomas Schatzl has updated the pull request incrementally with one additional
> commit since the last revision:
>
> iwalulya review
> * comments for variables tracking to-collection-set and just dirtied
> cards after GC/refinement
> * predicate for determining whether the refinement has been disabled
> * some other typos/comment improvements
> * renamed _has_xxx_ref to _has_ref_to_xxx to be more consistent with
> naming
src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp line 356:
> 354: bool do_heap_region(G1HeapRegion* r) override {
> 355: if (!r->is_free()) {
> 356: // Need to scan all parts of non-free regions, so reset the
> claim.
Why is the condition "is_free"? I thought we scan only old-or-humongous regions?
src/hotspot/share/gc/g1/g1ConcurrentRefine.hpp line 116:
> 114: SwapGlobalCT,// Swap global card table.
> 115: SwapJavaThreadsCT, // Swap java thread's card tables.
> 116: SwapGCThreadsCT, // Swap GC thread's card tables.
Do GC threads have card-table?
src/hotspot/share/gc/g1/g1ConcurrentRefineThread.cpp line 219:
> 217: // The young gen revising mechanism reads the predictor and the
> values set
> 218: // here. Avoid inconsistencies by locking.
> 219: MutexLocker x(G1RareEvent_lock, Mutex::_no_safepoint_check_flag);
Who else can be in this critical-section? I don't get what this lock is
protecting us from.
src/hotspot/share/gc/g1/g1ConcurrentRefineThread.hpp line 83:
> 81:
> 82: public:
> 83: static G1ConcurrentRefineThread* create(G1ConcurrentRefine* cr);
I wonder if the comment for this class "One or more G1 Concurrent Refinement
Threads..." has become obsolete. (AFAICS, this class is a singleton.)
src/hotspot/share/gc/g1/g1ConcurrentRefineWorkTask.cpp line 69:
> 67: } else if (res == G1RemSet::NoInteresting) {
> 68: _refine_stats.inc_cards_clean_again();
> 69: }
A `switch` is probably cleaner.
src/hotspot/share/gc/g1/g1ConcurrentRefineWorkTask.cpp line 78:
> 76: do_dirty_card(source, dest_card);
> 77: }
> 78: return pointer_delta(dirty_r, dirty_l, sizeof(CardValue));
I feel the `pointer_delta` line belongs to the caller. After that, even the
entire method can be inlined to the caller.
YMMV.
---
Re: RFR: 8343478: Remove unnecessary @SuppressWarnings annotations (core-libs) [v12]
On Sat, 15 Feb 2025 21:27:56 GMT, Archie Cobbs wrote: >> Please review this patch which removes unnecessary `@SuppressWarnings` >> annotations. > > Archie Cobbs has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 22 commits: > > - Merge branch 'master' into SuppressWarningsCleanup-core-libs to fix > conflicts. > - Remove a few more unnecessary suppressions. > - Merge branch 'master' into SuppressWarningsCleanup-core-libs > - Update @LastModified tags. > - Revert changes under src/java.desktop (to be moved into a separate PR). > - Bump copyright year to 2025. > - Merge branch 'master' into SuppressWarningsCleanup-core-libs > - Remove more unnecessary suppressions. > - Merge branch 'master' into SuppressWarningsCleanup-core-libs > - Remove more unnecessary @SuppressWarnings annotations. > - ... and 12 more: https://git.openjdk.org/jdk/compare/ba281196...340ca514 jpackage looks good - Marked as reviewed by asemenyuk (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21852#pullrequestreview-2658012030
Re: [External] : Re: RFR: 8350594: Misleading warning about install dir for DMG packaging
On 3/3/2025 9:58 PM, Michael Hall wrote: On Mar 3, 2025, at 1:51 PM, Michael Hall wrote: On Mar 3, 2025, at 1:37 PM, Alexey Semenyuk wrote: Hi Michael, Thank you for the detailed report! I filed [1] ticket that, I hope, captures all findings from your report. Let me know if anything was missed or misunderstood. [1] https://bugs.openjdk.org/browse/JDK-8351073 - Alexey Hello Alexey, jpackage should copy them as-is. It would be nice if this was the case but I think you need to be sure changes are made to the Info.plist to ensure the correct version is indicated in a few places. Apple offers something like plist buddy that could maybe be used. Fwiw, (base) mjh@MacBook-Pro-2 ~ % grep -C 1 CFBundleGetInfoString Info.plist libjli.dylib CFBundleGetInfoString Java SE 23.0.1+11-39 (base) mjh@MacBook-Pro-2 ~ % /usr/libexec/PlistBuddy -c 'set:CFBundleGetInfoString Java SE 25' Info.plist (base) mjh@MacBook-Pro-2 ~ % /usr/libexec/PlistBuddy -c 'save' Info.plist Saving... (base) mjh@MacBook-Pro-2 ~ % grep -C 1 CFBundleGetInfoString Info.plist libjli.dylib CFBundleGetInfoString Java SE 25 Good point about `CFBundleGetInfoString`. Maybe jpackage should set this field with the value of `--description` option. Another finding! As for editing "Info.plist" from the runtime source, if the directory referenced from `--runtime-image` option is a valid bundle, i.e. has "Contents/MacOS/libjli.dylib", "Contents/Info.plist" files, and "Contents/_CodeSignature" elements, and there are no other options, like `--app-version`, jpackage should use the supplied directory as-is without editing the existing "Info.plist" file. Otherwise, jpackage should create "Info.plist" and make sure the values are consistent, of course. - Alexey
Re: RFR: 8343478: Remove unnecessary @SuppressWarnings annotations (core-libs) [v12]
On Sat, 15 Feb 2025 21:27:56 GMT, Archie Cobbs wrote: >> Please review this patch which removes unnecessary `@SuppressWarnings` >> annotations. > > Archie Cobbs has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 22 commits: > > - Merge branch 'master' into SuppressWarningsCleanup-core-libs to fix > conflicts. > - Remove a few more unnecessary suppressions. > - Merge branch 'master' into SuppressWarningsCleanup-core-libs > - Update @LastModified tags. > - Revert changes under src/java.desktop (to be moved into a separate PR). > - Bump copyright year to 2025. > - Merge branch 'master' into SuppressWarningsCleanup-core-libs > - Remove more unnecessary suppressions. > - Merge branch 'master' into SuppressWarningsCleanup-core-libs > - Remove more unnecessary @SuppressWarnings annotations. > - ... and 12 more: https://git.openjdk.org/jdk/compare/ba281196...340ca514 Thanks for the reviews! - PR Comment: https://git.openjdk.org/jdk/pull/21852#issuecomment-2698092830
Re: RFR: 8350607: Consolidate MethodHandles::zero into MethodHandles::constant [v2]
On Mon, 24 Feb 2025 22:08:53 GMT, Chen Liang wrote: > we should just select a MH via hash table lookup and invoke that MH I had something like this in an early prototype of the `tableSwitch` combinator, but it does not work, as it prevents the method handle calls for each case from being inlined. - PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1979445701
Re: RFR: 8350617: Improve MethodHandles.tableSwitch and remove intrinsicData
On Tue, 4 Mar 2025 14:25:45 GMT, Jorn Vernee wrote: > as we skipped generating the intrinsic Huh, why? As I understand, intrinsics are bound on NamedFunction; the NamedFunction to select a MH should be still there in a MH chain. - PR Comment: https://git.openjdk.org/jdk/pull/23763#issuecomment-2697834960
Re: Question about IO.println
Sorry, forgot to link the thread. https://old.reddit.com/r/java/comments/1j2pr78/ And here is the comment in question. https://old.reddit.com/r/java/comments/1j2pr78/compact_source_files_and_instance_main_methods_jep/mfy9dof/
Re: RFR: 8350607: Consolidate MethodHandles::zero into MethodHandles::constant [v2]
On Mon, 24 Feb 2025 23:45:37 GMT, Chen Liang wrote:
>> LF editor spins classes, this avoids the spinning overhead and should speed
>> up non-capturing lambdas too.
>>
>> There may need to be additional intrinsic work for MH combinator lf bytecode
>> generation.
>
> Chen Liang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> We no longer load DelegateMH as we no longer rebind
src/java.base/share/classes/java/lang/invoke/LambdaForm.java line 1658:
> 1656: var carrier = argument(0, L_TYPE).withConstraint(species); //
> BMH bound with data
> 1657: Name[] constNames = new Name[] { carrier, new
> Name(species.getterFunction(0), carrier) };
> 1658: return LF_constant[type.ordinal()] = create(1, constNames,
> Kind.CONSTANT);
I think this caching logic should be in `constantForm`, which also does the
cache lookup.
src/java.base/share/classes/java/lang/invoke/LambdaFormEditor.java line 972:
> 970: callFilter = null;
> 971: else
> 972: callFilter = new Name(LambdaForm.identity(newType),
> newType.btWrapper.zero());
Maybe we could have a `Name` constructor that just takes an `Object` and
returns it like this.
test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestDynamicRegenerateHolderClasses.java
line 42:
> 40: static String CHECK_MESSAGES[] = {"java.lang.invoke.Invokers$Holder
> source: shared objects file (top)",
> 41:
> "java.lang.invoke.DirectMethodHandle$Holder source: shared objects file
> (top)",
> 42:
> "java.lang.invoke.DelegatingMethodHandle$Holder source: shared objects file
> (top)",
I don't understand why this change is needed. Could you please explain it?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1979282388
PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1979441441
PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1979454377
Re: RFR: 8350607: Consolidate MethodHandles::zero into MethodHandles::constant [v2]
On Tue, 4 Mar 2025 14:09:03 GMT, Chen Liang wrote:
>> src/java.base/share/classes/java/lang/invoke/LambdaForm.java line 1658:
>>
>>> 1656: var carrier = argument(0, L_TYPE).withConstraint(species); //
>>> BMH bound with data
>>> 1657: Name[] constNames = new Name[] { carrier, new
>>> Name(species.getterFunction(0), carrier) };
>>> 1658: return LF_constant[type.ordinal()] = create(1, constNames,
>>> Kind.CONSTANT);
>>
>> I think this caching logic should be in `constantForm`, which also does the
>> cache lookup.
>
> Putting the field write in the `create` method is to help reduce the code
> that JIT needs to compile by reducing the getter's code size.
Fair enough.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1979606038
Re: RFR: 8350607: Consolidate MethodHandles::zero into MethodHandles::constant [v2]
On Mon, 24 Feb 2025 23:45:37 GMT, Chen Liang wrote: >> LF editor spins classes, this avoids the spinning overhead and should speed >> up non-capturing lambdas too. >> >> There may need to be additional intrinsic work for MH combinator lf bytecode >> generation. > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > We no longer load DelegateMH as we no longer rebind Marked as reviewed by jvernee (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/23706#pullrequestreview-2657855612
Question about IO.println
Hello Amber Dev Team and Core Libs Dev Team, Another reddit discussion popped up today about IO.println(), and one of the comments mentioned that they wanted something along the lines of this. IO.println(num1, num2, num3); //prints out "1 2 3" I proposed that maybe a better option would be a String.join overload. So, instead of String.join(String delim, String... elements), there would be String.join(String delim, Object... elements), with each object getting toString() called on it. What are your thoughts on either of these ideas? I actually think the IO.println() version is nice, but I felt like String.join made a slightly better compromise. Another commentor mentioned that this will be easier with String Templates too, so maybe it is better to wait for that. I don't think String templates are a bad idea here, but it also felt like overkill imo. A library function seems like a better fit. Thank you for your time and thoughts. David Alayachew
Re: RFR: 8350617: Improve MethodHandles.tableSwitch and remove intrinsicData
On Tue, 4 Mar 2025 14:12:05 GMT, Jorn Vernee wrote: > the LambdaForm will just have a single call site for all the cases, whereas > the intrinsic does emit a call per case? Yes. The LambdaForm will be only used for non-customized bytecode (which cannot fully inline anyways) or interpretation. > i.e. the point is that these do not necessarily have to do exactly the same > thing? I think so - that is what these MH Impl intrinsics are for, to stub out some names with replacement bytecode. Though in the long run, we might check if it's possible and worthy to add library call (intrinsic) for our combinator and improve the IR for C2 directly. For performance, as I've mentioned before, JMH benchmarks results are the same for before and after; the only difference is that I had to add `@ForceInline` on Immutable collections to make them as performant as the old switch holder class because of the inlining shenigans when something is not constant. - PR Comment: https://git.openjdk.org/jdk/pull/23763#issuecomment-2697796793
Re: RFR: 8350607: Consolidate MethodHandles::zero into MethodHandles::constant [v2]
On Tue, 4 Mar 2025 11:56:50 GMT, Jorn Vernee wrote:
>> Chen Liang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> We no longer load DelegateMH as we no longer rebind
>
> src/java.base/share/classes/java/lang/invoke/LambdaForm.java line 1658:
>
>> 1656: var carrier = argument(0, L_TYPE).withConstraint(species); //
>> BMH bound with data
>> 1657: Name[] constNames = new Name[] { carrier, new
>> Name(species.getterFunction(0), carrier) };
>> 1658: return LF_constant[type.ordinal()] = create(1, constNames,
>> Kind.CONSTANT);
>
> I think this caching logic should be in `constantForm`, which also does the
> cache lookup.
Putting the field write in the `create` method is to help reduce the code that
JIT needs to compile by reducing the getter's code size.
> src/java.base/share/classes/java/lang/invoke/LambdaFormEditor.java line 972:
>
>> 970: callFilter = null;
>> 971: else
>> 972: callFilter = new Name(LambdaForm.identity(newType),
>> newType.btWrapper.zero());
>
> Maybe we could have a `Name` constructor that just takes an `Object` and
> returns it like this.
I don't think that is necessary - the majority of constant values like this are
immediately sent to a NamedFunction, which already accepts such constants
natively. The only case such a Name is necessary is for LF return values.
> test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestDynamicRegenerateHolderClasses.java
> line 42:
>
>> 40: static String CHECK_MESSAGES[] = {"java.lang.invoke.Invokers$Holder
>> source: shared objects file (top)",
>> 41:
>> "java.lang.invoke.DirectMethodHandle$Holder source: shared objects file
>> (top)",
>> 42:
>> "java.lang.invoke.DelegatingMethodHandle$Holder source: shared objects file
>> (top)",
>
> I don't understand why this change is needed. Could you please explain it?
With the simplification of `MethodHandles.constant`, it seems invoking
`MethodHandles.constant` no longer need to initialize `DelegatingMethodHandle`
because there is no longer need to rebind some MethodHandle; anyways we are
loading fewer classes now.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1979534142
PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1979537500
PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1979543546
Re: RFR: 8350607: Consolidate MethodHandles::zero into MethodHandles::constant [v2]
On Tue, 4 Mar 2025 13:35:50 GMT, Jorn Vernee wrote: >> I reviewed the other use of `intrinsicData`, `tableSwitch`. I believe the >> intrinsic is actually a regression by growing the bytecode size - we should >> just select a MH via hash table lookup and invoke that MH, given all MHs in >> that list have the same type. I have removed the use of intrinsic data here >> and we can move on to remove it later. >> >> I noticed that intrinsics are useful really only as part of named functions. >> And named functions only reuse arbitrary MHs for the invoker form. Is my >> understanding here correct? > >> we should just select a MH via hash table lookup and invoke that MH > > I had something like this in an early prototype of the `tableSwitch` > combinator, but it does not work, as it prevents the method handle calls for > each case from being inlined. FYI this is being addressed in #23763 - PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1979539450
Re: RFR: 8350617: Improve MethodHandles.tableSwitch and remove intrinsicData
On Tue, 25 Feb 2025 02:45:46 GMT, Chen Liang wrote: > The existing tableSwitch combinator's LF is unnecessarily complex. This patch > also simplifies the tableSwitch combinator. You're gonna have to explain this. Looking at the code, I think the optimization here is that, the LambdaForm will just have a single call site for all the cases, whereas the intrinsic does emit a call per case? i.e. the point is that these do not necessarily have to do exactly the same thing? I remember that having a call site per case was important from implementing this, since even if the selector is not a constant, we can then still inline each individual call site. Whereas with a single shared call site, the method handle would become not constant. - PR Comment: https://git.openjdk.org/jdk/pull/23763#issuecomment-2697769525
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v11]
On Tue, 4 Mar 2025 15:16:17 GMT, Albert Mingkun Yang wrote:
>> Thomas Schatzl has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> iwalulya review
>> * comments for variables tracking to-collection-set and just dirtied
>> cards after GC/refinement
>> * predicate for determining whether the refinement has been disabled
>> * some other typos/comment improvements
>> * renamed _has_xxx_ref to _has_ref_to_xxx to be more consistent with
>> naming
>
> src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp line 356:
>
>> 354: bool do_heap_region(G1HeapRegion* r) override {
>> 355: if (!r->is_free()) {
>> 356: // Need to scan all parts of non-free regions, so reset the
>> claim.
>
> Why is the condition "is_free"? I thought we scan only old-or-humongous
> regions?
We also need to clear young gen region marks because we want them to be all
clean in the card table for the garbage collection (evacuation failure
handling, use in next cycle).
This is maybe a bit of a waste if there are multiple refinement rounds between
two gcs, but it's less expensive than in the pause wrt to latency. It's fast
anyway.
> src/hotspot/share/gc/g1/g1ConcurrentRefine.hpp line 116:
>
>> 114: SwapGlobalCT,// Swap global card table.
>> 115: SwapJavaThreadsCT, // Swap java thread's card tables.
>> 116: SwapGCThreadsCT, // Swap GC thread's card tables.
>
> Do GC threads have card-table?
Hmm, I thought I changed tat already just recently with Ivan's latest requests.
Will fix.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23739#discussion_r1979742662
PR Review Comment: https://git.openjdk.org/jdk/pull/23739#discussion_r1979752692
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v11]
On Tue, 4 Mar 2025 15:33:29 GMT, Albert Mingkun Yang wrote: >> Thomas Schatzl has updated the pull request incrementally with one >> additional commit since the last revision: >> >> iwalulya review >> * comments for variables tracking to-collection-set and just dirtied >> cards after GC/refinement >> * predicate for determining whether the refinement has been disabled >> * some other typos/comment improvements >> * renamed _has_xxx_ref to _has_ref_to_xxx to be more consistent with >> naming > > src/hotspot/share/gc/g1/g1ConcurrentRefineThread.cpp line 219: > >> 217: // The young gen revising mechanism reads the predictor and the >> values set >> 218: // here. Avoid inconsistencies by locking. >> 219: MutexLocker x(G1RareEvent_lock, Mutex::_no_safepoint_check_flag); > > Who else can be in this critical-section? I don't get what this lock is > protecting us from. The concurrent refine control thread in `G1ConcurrentRefineThread::do_refinement`, when calling `G1Policy::record_dirtying_stats`. - PR Review Comment: https://git.openjdk.org/jdk/pull/23739#discussion_r1979759329
Integrated: 4745837: Make grouping usage during parsing apparent in relevant NumberFormat methods
On Wed, 26 Feb 2025 22:18:17 GMT, Justin Lu wrote: > Please review this PR which clarifies some behavior regarding NumberFormat > grouping specifically in the grouping related methods. > > Please see the corresponding CSR for further detail. Note that an alternative > would be to specify this at the DecimalFormat level, allowing NumberFormat > subclasses to define this behavior how they want. IMO, I would expect > `setGroupingUsed(boolean)` to affect both; a subclass could define > `(is|set)(Parsing|Formatting)GroupingUsed` if need be, thus the proposed > solution. This pull request has now been integrated. Changeset: 5b8d3491 Author:Justin Lu URL: https://git.openjdk.org/jdk/commit/5b8d3491bf685a64b72b0ae763697353d09f61a1 Stats: 14 lines in 1 file changed: 9 ins; 0 del; 5 mod 4745837: Make grouping usage during parsing apparent in relevant NumberFormat methods Reviewed-by: naoto - PR: https://git.openjdk.org/jdk/pull/23813
Re: RFR: 8351074: Disallow null prefix and suffix in DecimalFormat [v3]
On Tue, 4 Mar 2025 18:12:48 GMT, Justin Lu wrote: >> Please review this PR and associated CSR which disallows passing null to 4 >> `DecimalFormat` prefix/suffix setter methods. >> >> Currently these setters do not check the input String for null. When the >> prefix/suffix is null, any such DecimalFormat instances are effectively >> non-functional as it will throw NPE for most operations. > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > Make non-null apparent in param tag as well LGTM - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23880#pullrequestreview-2658576784
Re: RFR: 8350617: Improve MethodHandles.tableSwitch and remove intrinsicData
On Tue, 4 Mar 2025 13:42:16 GMT, Jorn Vernee wrote: >> Remove the intrinsicData field. We can obtain this from the customized MH >> when we spin ultra-customized lambda forms. In the long run, we aim to >> remove this intrinsic if there is no regression for call site sharing. >> >> The existing tableSwitch combinator's LF is unnecessarily complex. This >> patch also simplifies the tableSwitch combinator. >> >> Note that I was forced to add `@ForceInline` on immutable lists due to >> regressions in `MethodHandlesTableSwitchRandom` with `sorted == true`, which >> eliminates the regression. Otherwise, all benchmark results are the same. >> Tested java/lang/invoke. > > src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java > line 628: > >> 626: continue; >> 627: } >> 628: break; // Only inline target MHs if >> this is customized > > I think this could be problematic, as we typically only customize the root > method handle in a chain. So, if a table switch handle is used with another > combinator, we will never benefit from the intrinsic. In that case, won't the root form be customized and the table switch names be inlined into the root form? - PR Review Comment: https://git.openjdk.org/jdk/pull/23763#discussion_r1979571760
Re: RFR: 8350617: Improve MethodHandles.tableSwitch and remove intrinsicData
On Tue, 25 Feb 2025 02:45:46 GMT, Chen Liang wrote: > Remove the intrinsicData field. We can obtain this from the customized MH > when we spin ultra-customized lambda forms. In the long run, we aim to remove > this intrinsic if there is no regression for call site sharing. > > The existing tableSwitch combinator's LF is unnecessarily complex. This patch > also simplifies the tableSwitch combinator. > > Note that I was forced to add `@ForceInline` on immutable lists due to > regressions in `MethodHandlesTableSwitchRandom` with `sorted == true`, which > eliminates the regression. Otherwise, all benchmark results are the same. > Tested java/lang/invoke. src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line 628: > 626: continue; > 627: } > 628: break; // Only inline target MHs if > this is customized I think this could be problematic, as we typically only customize the root method handle in a chain. So, if a table switch handle is used with another combinator, we will never benefit from the intrinsic. src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 7664: > 7662: if (mh.type() != expectedType) > 7663: throw new IllegalArgumentException( > 7664: "Some targets do not have the expected type " + > expectedType + ": " + caseActions); I think we should avoid changing the exception messages here, since it's observable from the outside. FWIW, one of the downsides of only printing out a single method handle, is that a user can't tell which method handle in the list was problematic. - PR Review Comment: https://git.openjdk.org/jdk/pull/23763#discussion_r1979460526 PR Review Comment: https://git.openjdk.org/jdk/pull/23763#discussion_r1979537776
Re: RFR: 8350617: Improve MethodHandles.tableSwitch and remove intrinsicData
On Tue, 4 Mar 2025 14:11:28 GMT, Jorn Vernee wrote: >> src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 7664: >> >>> 7662: if (mh.type() != expectedType) >>> 7663: throw new IllegalArgumentException( >>> 7664: "Some targets do not have the expected type " + >>> expectedType + ": " + caseActions); >> >> I think we should avoid changing the exception messages here, since it's >> observable from the outside. >> >> FWIW, one of the downsides of only printing out a single method handle, is >> that a user can't tell which method handle in the list was problematic. > > At least, please motivate these changes. Why do you think changing the > message is needed? Here you are making the check against the default case. However, these messages never print the default case's type, making this error message uninformative. - PR Review Comment: https://git.openjdk.org/jdk/pull/23763#discussion_r1979575675
Re: RFR: 8350617: Improve MethodHandles.tableSwitch and remove intrinsicData
On Tue, 4 Mar 2025 14:33:27 GMT, Jorn Vernee wrote: >> In that case, won't the root form be customized and the table switch names >> be inlined into the root form? > > The root would e.g. have a constant BMH pointing at a _shared_ tableSwitch > LF. So, the BMH fields would be seen as constant as well, including the list > of cases. The issue is that the bytecode of the tableSwitch BMH will not be > using the intrinsic, since it's not customized itself, i.e. it will only have > a single call site for all cases. I think we could get around this by always wrapping the `tableSwitch` MH in an `exactInvoker`, which adds the customization check for its callee. It would mean we will also separately customize any tableSwitch MH that's part of a larger chain. - PR Review Comment: https://git.openjdk.org/jdk/pull/23763#discussion_r1979591207
Re: RFR: 8350617: Improve MethodHandles.tableSwitch and remove intrinsicData
On Tue, 4 Mar 2025 14:10:36 GMT, Jorn Vernee wrote: >> Remove the intrinsicData field. We can obtain this from the customized MH >> when we spin ultra-customized lambda forms. In the long run, we aim to >> remove this intrinsic if there is no regression for call site sharing. >> >> The existing tableSwitch combinator's LF is unnecessarily complex. This >> patch also simplifies the tableSwitch combinator. >> >> Note that I was forced to add `@ForceInline` on immutable lists due to >> regressions in `MethodHandlesTableSwitchRandom` with `sorted == true`, which >> eliminates the regression. Otherwise, all benchmark results are the same. >> Tested java/lang/invoke. > > src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 7664: > >> 7662: if (mh.type() != expectedType) >> 7663: throw new IllegalArgumentException( >> 7664: "Some targets do not have the expected type " + >> expectedType + ": " + caseActions); > > I think we should avoid changing the exception messages here, since it's > observable from the outside. > > FWIW, one of the downsides of only printing out a single method handle, is > that a user can't tell which method handle in the list was problematic. At least, please motivate these changes. Why do you think changing the message is needed? - PR Review Comment: https://git.openjdk.org/jdk/pull/23763#discussion_r1979539878
Re: RFR: 8349358: [JMH] Cannot access class jdk.internal.vm.ContinuationScope
On Tue, 4 Feb 2025 12:35:50 GMT, SendaoYan wrote: > Hi all, > > Several JMH tests fails "cannot access class > jdk.internal.vm.ContinuationScope (in module java.base) because module > java.base does not export jdk.internal.vm to unnamed module". This PR add VM > option `--add-exports=java.base/jdk.internal.vm=ALL-UNNAMED` to fix the JMH > test bug. > > Change has been verified locally, test-fix only, no risk. Should we remove these JMH tests, or just fix the test bug and make tests run normally - PR Comment: https://git.openjdk.org/jdk/pull/23437#issuecomment-2698041624
Re: RFR: 8350607: Consolidate MethodHandles::zero into MethodHandles::constant [v2]
On Tue, 25 Feb 2025 21:51:15 GMT, Claes Redestad wrote: >> Chen Liang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> We no longer load DelegateMH as we no longer rebind > > Just got back home. Some comments inline, will need to run some tests and > mull this over before approval. @cl4es @rose00 I think I have addressed your existing comments; can you review again? - PR Comment: https://git.openjdk.org/jdk/pull/23706#issuecomment-2698472891
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v12]
> Hi all, > > please review this change that implements (currently Draft) JEP: G1: > Improve Application Throughput with a More Efficient Write-Barrier. > > The reason for posting this early is that this is a large change, and the JEP > process is already taking very long with no end in sight but we would like to > have this ready by JDK 25. > > ### Current situation > > With this change, G1 will reduce the post write barrier to much more resemble > Parallel GC's as described in the JEP. The reason is that G1 lacks in > throughput compared to Parallel/Serial GC due to larger barrier. > > The main reason for the current barrier is how g1 implements concurrent > refinement: > * g1 tracks dirtied cards using sets (dirty card queue set - dcqs) of buffers > (dirty card queues - dcq) containing the location of dirtied cards. > Refinement threads pick up their contents to re-refine. The barrier needs to > enqueue card locations. > * For correctness dirty card updates requires fine-grained synchronization > between mutator and refinement threads, > * Finally there is generic code to avoid dirtying cards altogether (filters), > to avoid executing the synchronization and the enqueuing as much as possible. > > These tasks require the current barrier to look as follows for an assignment > `x.a = y` in pseudo code: > > > // Filtering > if (region(@x.a) == region(y)) goto done; // same region check > if (y == null) goto done; // null value check > if (card(@x.a) == young_card) goto done; // write to young gen check > StoreLoad;// synchronize > if (card(@x.a) == dirty_card) goto done; > > *card(@x.a) = dirty > > // Card tracking > enqueue(card-address(@x.a)) into thread-local-dcq; > if (thread-local-dcq is not full) goto done; > > call runtime to move thread-local-dcq into dcqs > > done: > > > Overall this post-write barrier alone is in the range of 40-50 total > instructions, compared to three or four(!) for parallel and serial gc. > > The large size of the inlined barrier not only has a large code footprint, > but also prevents some compiler optimizations like loop unrolling or inlining. > > There are several papers showing that this barrier alone can decrease > throughput by 10-20% > ([Yang12](https://dl.acm.org/doi/10.1145/2426642.2259004)), which is > corroborated by some benchmarks (see links). > > The main idea for this change is to not use fine-grained synchronization > between refinement and mutator threads, but coarse grained based on > atomically switching card tables. Mutators only work on the "primary" card > table, refinement threads on a se... Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: ayang review * renamings * refactorings - Changes: - all: https://git.openjdk.org/jdk/pull/23739/files - new: https://git.openjdk.org/jdk/pull/23739/files/b4d19d9b..4a978118 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23739&range=11 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23739&range=10-11 Stats: 34 lines in 4 files changed: 13 ins; 1 del; 20 mod Patch: https://git.openjdk.org/jdk/pull/23739.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23739/head:pull/23739 PR: https://git.openjdk.org/jdk/pull/23739
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v11]
On Tue, 4 Mar 2025 16:00:46 GMT, Thomas Schatzl wrote: >> src/hotspot/share/gc/g1/g1ConcurrentRefine.hpp line 116: >> >>> 114: SwapGlobalCT,// Swap global card table. >>> 115: SwapJavaThreadsCT, // Swap java thread's card tables. >>> 116: SwapGCThreadsCT, // Swap GC thread's card tables. >> >> Do GC threads have card-table? > > Hmm, I thought I changed tat already just recently with Ivan's latest > requests. Will fix. Oh, I only fixed the string. Apologies. - PR Review Comment: https://git.openjdk.org/jdk/pull/23739#discussion_r1979761737
Re: [External] : Re: RFR: 8350594: Misleading warning about install dir for DMG packaging
On 3/4/2025 11:06 AM, Michael Hall wrote: Good point about `CFBundleGetInfoString`. Maybe jpackage should set this field with the value of `--description` option. Another finding! As for editing "Info.plist" from the runtime source, if the directory referenced from `--runtime-image` option is a valid bundle, i.e. has "Contents/MacOS/libjli.dylib", "Contents/Info.plist" files, and "Contents/_CodeSignature" elements, and there are no other options, like `--app-version`, jpackage should use the supplied directory as-is without editing the existing "Info.plist" file. Otherwise, jpackage should create "Info.plist" and make sure the values are consistent, of course. I posted an example, there are a few other places in Info.plist that would also need changing to the correct version if you don’t already have a valid Info.plist for the JRE. The page of mine that you linked I think mentioned them all. __CodeSignature I didn’t include in what I did. It seemed like you might not be able to copy this but would need to do your own signing to generate it for a different JRE? My guess was that it is only really needed for an application distribution including that JRE. I know jpackage does some of its own signing. If this recreates the JRE __CodeSignature, this probably wouldn’t be a concern. It is possible if you are provided with a JRE where all this is correct then copying as-is would be fine. Right. If you package unpacked official OpenJDK tar.gz jpackage should take it as is. However, if you are copying a JRE that already has a JavaVirtualMachines file for that version I’m not sure what would happen. The same as in the above case, I guess. jpackage should take it as is. That should be checked? If you are providing a later release the new one should be used. Sorry, I don't follow. What release do you mean? If you are providing an earlier release the one you are adding will be ignored. Warnings at least? /usr/libexec/java_home -v could maybe be useful here. I think though you would need to determine the version of what you are copying in. Assuming the JRE you are adding to JavaVirtualMachines is a unique or current one for that java version then I think some fairly simple test like java —version or something again with /usr/libexec/java_home could verify that everything is alright. jpackage doesn't control how you use bundles it produces. You can use the same input runtime image and create two separate bundles with different names, but with the same versions and install them both. - Alexey
Re: [External] : Re: RFR: 8350594: Misleading warning about install dir for DMG packaging
>> > Good point about `CFBundleGetInfoString`. Maybe jpackage should set this > field with the value of `--description` option. Another finding! > > As for editing "Info.plist" from the runtime source, if the directory > referenced from `--runtime-image` option is a valid bundle, i.e. has > "Contents/MacOS/libjli.dylib", "Contents/Info.plist" files, and > "Contents/_CodeSignature" elements, and there are no other options, like > `--app-version`, jpackage should use the supplied directory as-is without > editing the existing "Info.plist" file. Otherwise, jpackage should create > "Info.plist" and make sure the values are consistent, of course. I posted an example, there are a few other places in Info.plist that would also need changing to the correct version if you don’t already have a valid Info.plist for the JRE. The page of mine that you linked I think mentioned them all. __CodeSignature I didn’t include in what I did. It seemed like you might not be able to copy this but would need to do your own signing to generate it for a different JRE? My guess was that it is only really needed for an application distribution including that JRE. I know jpackage does some of its own signing. If this recreates the JRE __CodeSignature, this probably wouldn’t be a concern. It is possible if you are provided with a JRE where all this is correct then copying as-is would be fine. However, if you are copying a JRE that already has a JavaVirtualMachines file for that version I’m not sure what would happen. That should be checked? If you are providing a later release the new one should be used. If you are providing an earlier release the one you are adding will be ignored. Warnings at least? /usr/libexec/java_home -v could maybe be useful here. I think though you would need to determine the version of what you are copying in. Assuming the JRE you are adding to JavaVirtualMachines is a unique or current one for that java version then I think some fairly simple test like java —version or something again with /usr/libexec/java_home could verify that everything is alright.
Re: RFR: 8351074: Disallow null prefix and suffix in DecimalFormat [v3]
> Please review this PR and associated CSR which disallows passing null to 4 > `DecimalFormat` prefix/suffix setter methods. > > Currently these setters do not check the input String for null. When the > prefix/suffix is null, any such DecimalFormat instances are effectively > non-functional as it will throw NPE for most operations. Justin Lu has updated the pull request incrementally with one additional commit since the last revision: Make non-null apparent in param tag as well - Changes: - all: https://git.openjdk.org/jdk/pull/23880/files - new: https://git.openjdk.org/jdk/pull/23880/files/e13ac0d0..623bfdac Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23880&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23880&range=01-02 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/23880.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23880/head:pull/23880 PR: https://git.openjdk.org/jdk/pull/23880
Integrated: 8350594: Misleading warning about install dir for DMG packaging
On Mon, 24 Feb 2025 16:54:59 GMT, Alexey Semenyuk wrote: > - Fix the warning message about the custom installation directory location > jpackage issues for DMG runtime packaging. > - Don't issue the warning when the `--install-dir` value equals the default > installation directory location. > - Add relevant tests. This pull request has now been integrated. Changeset: 6a31aaeb Author:Alexey Semenyuk URL: https://git.openjdk.org/jdk/commit/6a31aaeb00b6c37e2e19c5f2759c4aa9ed87f25a Stats: 186 lines in 5 files changed: 154 ins; 19 del; 13 mod 8350594: Misleading warning about install dir for DMG packaging Reviewed-by: almatvee - PR: https://git.openjdk.org/jdk/pull/23752
Re: [External] : Re: RFR: 8350594: Misleading warning about install dir for DMG packaging
> On Mar 4, 2025, at 10:06 AM, Michael Hall wrote: > >>> >> Good point about `CFBundleGetInfoString`. Maybe jpackage should set this >> field with the value of `--description` option. Another finding! >> >> As for editing "Info.plist" from the runtime source, if the directory >> referenced from `--runtime-image` option is a valid bundle, i.e. has >> "Contents/MacOS/libjli.dylib", "Contents/Info.plist" files, and >> "Contents/_CodeSignature" elements, and there are no other options, like >> `--app-version`, jpackage should use the supplied directory as-is without >> editing the existing "Info.plist" file. Otherwise, jpackage should create >> "Info.plist" and make sure the values are consistent, of course. > > I posted an example, there are a few other places in Info.plist that would > also need changing to the correct version if you don’t already have a valid > Info.plist for the JRE. The page of mine that you linked I think mentioned > them all. > > __CodeSignature I didn’t include in what I did. It seemed like you might not > be able to copy this but would need to do your own signing to generate it for > a different JRE? My guess was that it is only really needed for an > application distribution including that JRE. I know jpackage does some of its > own signing. If this recreates the JRE __CodeSignature, this probably > wouldn’t be a concern. > > It is possible if you are provided with a JRE where all this is correct then > copying as-is would be fine. However, if you are copying a JRE that already > has a JavaVirtualMachines file for that version I’m not sure what would > happen. That should be checked? If you are providing a later release the new > one should be used. If you are providing an earlier release the one you are > adding will be ignored. Warnings at least? /usr/libexec/java_home -v > could maybe be useful here. I think though you would need to > determine the version of what you are copying in. > > Assuming the JRE you are adding to JavaVirtualMachines is a unique or current > one for that java version then I think some fairly simple test like java > —version or something again with /usr/libexec/java_home could verify that > everything is alright. > I didn’t really think that through. I just realized that if you have a dmg with a JRE that the user will install onto their own machine all the version related will depend on what they have in their own JavaVirtualMachines. You can’t really do any install checking on a drag and drop. So I guess that would be at their own risk. You could still do some verification on adding the JRE to a dmg to ensure its validity for the Mac specific files.
Re: RFR: 8344332: (bf) Migrate DirectByteBuffer to use java.lang.ref.Cleaner [v13]
On Sun, 23 Feb 2025 10:20:23 GMT, Kim Barrett wrote: > I've made a prototype that adds Cleaner.waitForCleaning() and changes > Bits.reserveMemory() to use it. > > https://github.com/openjdk/jdk/compare/master...kimbarrett:openjdk-jdk:wait-for-cleaning?expand=1 These caught my eye during my read-through: - The main `CleanerImpl.run()` loop is changed to call `queue.poll()` in place of (blocking) `queue.remove(timeout)`. Could this make this a busier wait loop? - My impression is that it does more locking. I wonder how these together could affect performance of less specialized Cleaner usage cases (where objects are not being enqueued very often). I'm therefore inclined for changes along these lines to be limited to a DirectByteBuffer-specific mechanism, as you suggested in your point (2). - PR Comment: https://git.openjdk.org/jdk/pull/22165#issuecomment-2699207286
Re: RFR: 8343191: Cgroup v1 subsystem fails to set subsystem path [v17]
On Fri, 28 Feb 2025 20:40:37 GMT, Sergey Chernyshev wrote: >> Cgroup V1 subsustem fails to initialize mounted controllers properly in >> certain cases, that may lead to controllers left undetected/inactive. We >> observed the behavior in CloudFoundry deployments, it affects also host >> systems. >> >> The relevant /proc/self/mountinfo line is >> >> >> 2207 2196 0:43 >> /system.slice/garden.service/garden/good/2f57368b-0eda-4e52-64d8-af5c >> /sys/fs/cgroup/cpu,cpuacct ro,nosuid,nodev,noexec,relatime master:25 - >> cgroup cgroup rw,cpu,cpuacct >> >> >> /proc/self/cgroup: >> >> >> 11:cpu,cpuacct:/system.slice/garden.service/garden/bad/2f57368b-0eda-4e52-64d8-af5c >> >> >> Here, Java runs inside containerized process that is being moved cgroups due >> to load balancing. >> >> Let's examine the condition at line 64 here >> https://github.com/openjdk/jdk/blob/55a7cf14453b6cd1de91362927b2fa63cba400a1/src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp#L59-L72 >> It is always FALSE and the branch is never taken. The issue was spotted >> earlier by @jerboaa in >> [JDK-8288019](https://bugs.openjdk.org/browse/JDK-8288019). >> >> The original logic was intended to find the common prefix of `_root`and >> `cgroup_path` and concatenate the remaining suffix to the `_mount_point` >> (lines 67-68). That could lead to the following results: >> >> Example input >> >> _root = "/a" >> cgroup_path = "/a/b" >> _mount_point = "/sys/fs/cgroup/cpu,cpuacct" >> >> >> result _path >> >> "/sys/fs/cgroup/cpu,cpuacct/b" >> >> >> Here, cgroup_path comes from /proc/self/cgroup 3rd column. The man page >> (https://man7.org/linux/man-pages/man7/cgroups.7.html#NOTES) for control >> groups states: >> >> >> ... >>/proc/pid/cgroup (since Linux 2.6.24) >> This file describes control groups to which the process >> with the corresponding PID belongs. The displayed >> information differs for cgroups version 1 and version 2 >> hierarchies. >> For each cgroup hierarchy of which the process is a >> member, there is one entry containing three colon- >> separated fields: >> >> hierarchy-ID:controller-list:cgroup-path >> >> For example: >> >> 5:cpuacct,cpu,cpuset:/daemons >> ... >> [3] This field contains the pathname of the control group >>in the hierarchy to which the process belongs. This >>pathname is relative to the mount point of the >>hierarchy. >> >> >> This explicitly states the "pathname is relative t... > > Sergey Chernyshev has updated the pull request incrementally with one > additional commit since the last revision: > > updated copyright headers Marked as reviewed by mbaesken (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21808#pullrequestreview-2656386374
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v9]
On Tue, 4 Mar 2025 09:57:56 GMT, Thomas Schatzl wrote: >> Hi all, >> >> please review this change that implements (currently Draft) JEP: G1: >> Improve Application Throughput with a More Efficient Write-Barrier. >> >> The reason for posting this early is that this is a large change, and the >> JEP process is already taking very long with no end in sight but we would >> like to have this ready by JDK 25. >> >> ### Current situation >> >> With this change, G1 will reduce the post write barrier to much more >> resemble Parallel GC's as described in the JEP. The reason is that G1 lacks >> in throughput compared to Parallel/Serial GC due to larger barrier. >> >> The main reason for the current barrier is how g1 implements concurrent >> refinement: >> * g1 tracks dirtied cards using sets (dirty card queue set - dcqs) of >> buffers (dirty card queues - dcq) containing the location of dirtied cards. >> Refinement threads pick up their contents to re-refine. The barrier needs to >> enqueue card locations. >> * For correctness dirty card updates requires fine-grained synchronization >> between mutator and refinement threads, >> * Finally there is generic code to avoid dirtying cards altogether >> (filters), to avoid executing the synchronization and the enqueuing as much >> as possible. >> >> These tasks require the current barrier to look as follows for an assignment >> `x.a = y` in pseudo code: >> >> >> // Filtering >> if (region(@x.a) == region(y)) goto done; // same region check >> if (y == null) goto done; // null value check >> if (card(@x.a) == young_card) goto done; // write to young gen check >> StoreLoad;// synchronize >> if (card(@x.a) == dirty_card) goto done; >> >> *card(@x.a) = dirty >> >> // Card tracking >> enqueue(card-address(@x.a)) into thread-local-dcq; >> if (thread-local-dcq is not full) goto done; >> >> call runtime to move thread-local-dcq into dcqs >> >> done: >> >> >> Overall this post-write barrier alone is in the range of 40-50 total >> instructions, compared to three or four(!) for parallel and serial gc. >> >> The large size of the inlined barrier not only has a large code footprint, >> but also prevents some compiler optimizations like loop unrolling or >> inlining. >> >> There are several papers showing that this barrier alone can decrease >> throughput by 10-20% >> ([Yang12](https://dl.acm.org/doi/10.1145/2426642.2259004)), which is >> corroborated by some benchmarks (see links). >> >> The main idea for this change is to not use fine-grained synchronization >> between refinement and mutator threads, but coarse grained based on >> atomically switching c... > > Thomas Schatzl has updated the pull request incrementally with one additional > commit since the last revision: > > * iwalulya review 2 > * G1ConcurrentRefineWorkState -> G1ConcurrentRefineSweepState > * some additional documentation I got an error while testing java/foreign/TestUpcallStress.java on linuxaarch64 with this PR: # Internal Error (/openjdk-jdk-linux_aarch64-dbg/jdk/src/hotspot/share/gc/g1/g1CardTable.cpp:56), pid=19044, tid=19159 # guarantee(!failures) failed: there should not have been any failures ... V [libjvm.so+0xb6e988] G1CardTable::verify_region(MemRegion, unsigned char, bool)+0x3b8 (g1CardTable.cpp:56) V [libjvm.so+0xc3a10c] G1MergeHeapRootsTask::G1ClearBitmapClosure::do_heap_region(G1HeapRegion*)+0x13c (g1RemSet.cpp:1048) V [libjvm.so+0xb7a80c] G1CollectedHeap::par_iterate_regions_array(G1HeapRegionClosure*, G1HeapRegionClaimer*, unsigned int const*, unsigned long, unsigned int) const+0x9c (g1CollectedHeap.cpp:2059) V [libjvm.so+0xc49fe8] G1MergeHeapRootsTask::work(unsigned int)+0x708 (g1RemSet.cpp:1225) V [libjvm.so+0x19597bc] WorkerThread::run()+0x98 (workerThread.cpp:69) V [libjvm.so+0x1824510] Thread::call_run()+0xac (thread.cpp:231) V [libjvm.so+0x13b3994] thread_native_entry(Thread*)+0x130 (os_linux.cpp:877) C [libpthread.so.0+0x875c] start_thread+0x18c - PR Comment: https://git.openjdk.org/jdk/pull/23739#issuecomment-2697024679
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v9]
On Tue, 4 Mar 2025 10:37:47 GMT, Martin Doerr wrote: > I got an error while testing java/foreign/TestUpcallStress.java on > linuxaarch64 with this PR: > > ``` > # Internal Error > (/openjdk-jdk-linux_aarch64-dbg/jdk/src/hotspot/share/gc/g1/g1CardTable.cpp:56), > pid=19044, tid=19159 > # guarantee(!failures) failed: there should not have been any failures > ... > V [libjvm.so+0xb6e988] G1CardTable::verify_region(MemRegion, unsigned char, > bool)+0x3b8 (g1CardTable.cpp:56) > V [libjvm.so+0xc3a10c] > G1MergeHeapRootsTask::G1ClearBitmapClosure::do_heap_region(G1HeapRegion*)+0x13c > (g1RemSet.cpp:1048) > V [libjvm.so+0xb7a80c] > G1CollectedHeap::par_iterate_regions_array(G1HeapRegionClosure*, > G1HeapRegionClaimer*, unsigned int const*, unsigned long, unsigned int) > const+0x9c (g1CollectedHeap.cpp:2059) > V [libjvm.so+0xc49fe8] G1MergeHeapRootsTask::work(unsigned int)+0x708 > (g1RemSet.cpp:1225) > V [libjvm.so+0x19597bc] WorkerThread::run()+0x98 (workerThread.cpp:69) > V [libjvm.so+0x1824510] Thread::call_run()+0xac (thread.cpp:231) > V [libjvm.so+0x13b3994] thread_native_entry(Thread*)+0x130 > (os_linux.cpp:877) > C [libpthread.so.0+0x875c] start_thread+0x18c > ``` I will try to reproduce. Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/23739#issuecomment-2697052899
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v5]
On Tue, 4 Mar 2025 08:36:58 GMT, Thomas Schatzl wrote: >> `cards_still_refer_to_cset` refers to cards that were found to have already >> been marked as `to-collection-set`. Renamed to >> `_cards_already_refer_to_cset`, would that be okay? > > Fwiw, this particular counter is just for statistics, so if you want I can > remove these. I did some experiments with re-examining these cards too to see > whether we could clear them later. For determining if/when to do that a rate > of increase for the young cards has been interesting. > > As mentioned, if you want I can remove them. `_cards_already_refer_to_cset` is fine by me, i don't like the option of removing them - PR Review Comment: https://git.openjdk.org/jdk/pull/23739#discussion_r1979009507
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v5]
On Tue, 4 Mar 2025 08:26:10 GMT, Thomas Schatzl wrote: >> src/hotspot/share/gc/g1/g1CollectionSet.cpp line 310: >> >>> 308: verify_young_cset_indices(); >>> 309: >>> 310: size_t card_rs_length = >>> _policy->analytics()->predict_card_rs_length(in_young_only_phase); >> >> Why are we using a prediction here? Additionally, won't this prediction also >> include cards from the old gen regions in case of mixed gcs? How do we >> reconcile that when we are adding old gen regions to c-set? > >> Why are we using a prediction here? > > Quickly checking again, do we have the actual count here from somewhere? > >> Additionally, won't this prediction also include cards from the old gen >> regions in case of mixed gcs? How do we reconcile that when we are adding >> old gen regions to c-set? > > The predictor contents changed to (supposedly) only contain cards containing > young gen references. See g1Policy.cpp:934: > > _analytics->report_card_rs_length(total_cards_scanned - > total_non_young_rs_cards, is_young_only_pause); Fair, I missed that details on young RS have been removed. - PR Review Comment: https://git.openjdk.org/jdk/pull/23739#discussion_r1979022900
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v8]
> Hi all, > > please review this change that implements (currently Draft) JEP: G1: > Improve Application Throughput with a More Efficient Write-Barrier. > > The reason for posting this early is that this is a large change, and the JEP > process is already taking very long with no end in sight but we would like to > have this ready by JDK 25. > > ### Current situation > > With this change, G1 will reduce the post write barrier to much more resemble > Parallel GC's as described in the JEP. The reason is that G1 lacks in > throughput compared to Parallel/Serial GC due to larger barrier. > > The main reason for the current barrier is how g1 implements concurrent > refinement: > * g1 tracks dirtied cards using sets (dirty card queue set - dcqs) of buffers > (dirty card queues - dcq) containing the location of dirtied cards. > Refinement threads pick up their contents to re-refine. The barrier needs to > enqueue card locations. > * For correctness dirty card updates requires fine-grained synchronization > between mutator and refinement threads, > * Finally there is generic code to avoid dirtying cards altogether (filters), > to avoid executing the synchronization and the enqueuing as much as possible. > > These tasks require the current barrier to look as follows for an assignment > `x.a = y` in pseudo code: > > > // Filtering > if (region(@x.a) == region(y)) goto done; // same region check > if (y == null) goto done; // null value check > if (card(@x.a) == young_card) goto done; // write to young gen check > StoreLoad;// synchronize > if (card(@x.a) == dirty_card) goto done; > > *card(@x.a) = dirty > > // Card tracking > enqueue(card-address(@x.a)) into thread-local-dcq; > if (thread-local-dcq is not full) goto done; > > call runtime to move thread-local-dcq into dcqs > > done: > > > Overall this post-write barrier alone is in the range of 40-50 total > instructions, compared to three or four(!) for parallel and serial gc. > > The large size of the inlined barrier not only has a large code footprint, > but also prevents some compiler optimizations like loop unrolling or inlining. > > There are several papers showing that this barrier alone can decrease > throughput by 10-20% > ([Yang12](https://dl.acm.org/doi/10.1145/2426642.2259004)), which is > corroborated by some benchmarks (see links). > > The main idea for this change is to not use fine-grained synchronization > between refinement and mutator threads, but coarse grained based on > atomically switching card tables. Mutators only work on the "primary" card > table, refinement threads on a se... Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: * do not change card table base for gc threads during swapping * not necessary because they do not use it * (recent assert that verifies that non-java threads do not have a card table found this) - Changes: - all: https://git.openjdk.org/jdk/pull/23739/files - new: https://git.openjdk.org/jdk/pull/23739/files/8f46dc9a..9e2ee543 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23739&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23739&range=06-07 Stats: 25 lines in 1 file changed: 9 ins; 14 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/23739.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23739/head:pull/23739 PR: https://git.openjdk.org/jdk/pull/23739
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v9]
> Hi all, > > please review this change that implements (currently Draft) JEP: G1: > Improve Application Throughput with a More Efficient Write-Barrier. > > The reason for posting this early is that this is a large change, and the JEP > process is already taking very long with no end in sight but we would like to > have this ready by JDK 25. > > ### Current situation > > With this change, G1 will reduce the post write barrier to much more resemble > Parallel GC's as described in the JEP. The reason is that G1 lacks in > throughput compared to Parallel/Serial GC due to larger barrier. > > The main reason for the current barrier is how g1 implements concurrent > refinement: > * g1 tracks dirtied cards using sets (dirty card queue set - dcqs) of buffers > (dirty card queues - dcq) containing the location of dirtied cards. > Refinement threads pick up their contents to re-refine. The barrier needs to > enqueue card locations. > * For correctness dirty card updates requires fine-grained synchronization > between mutator and refinement threads, > * Finally there is generic code to avoid dirtying cards altogether (filters), > to avoid executing the synchronization and the enqueuing as much as possible. > > These tasks require the current barrier to look as follows for an assignment > `x.a = y` in pseudo code: > > > // Filtering > if (region(@x.a) == region(y)) goto done; // same region check > if (y == null) goto done; // null value check > if (card(@x.a) == young_card) goto done; // write to young gen check > StoreLoad;// synchronize > if (card(@x.a) == dirty_card) goto done; > > *card(@x.a) = dirty > > // Card tracking > enqueue(card-address(@x.a)) into thread-local-dcq; > if (thread-local-dcq is not full) goto done; > > call runtime to move thread-local-dcq into dcqs > > done: > > > Overall this post-write barrier alone is in the range of 40-50 total > instructions, compared to three or four(!) for parallel and serial gc. > > The large size of the inlined barrier not only has a large code footprint, > but also prevents some compiler optimizations like loop unrolling or inlining. > > There are several papers showing that this barrier alone can decrease > throughput by 10-20% > ([Yang12](https://dl.acm.org/doi/10.1145/2426642.2259004)), which is > corroborated by some benchmarks (see links). > > The main idea for this change is to not use fine-grained synchronization > between refinement and mutator threads, but coarse grained based on > atomically switching card tables. Mutators only work on the "primary" card > table, refinement threads on a se... Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: * iwalulya review 2 * G1ConcurrentRefineWorkState -> G1ConcurrentRefineSweepState * some additional documentation - Changes: - all: https://git.openjdk.org/jdk/pull/23739/files - new: https://git.openjdk.org/jdk/pull/23739/files/9e2ee543..442d9eae Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23739&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23739&range=07-08 Stats: 93 lines in 7 files changed: 27 ins; 3 del; 63 mod Patch: https://git.openjdk.org/jdk/pull/23739.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23739/head:pull/23739 PR: https://git.openjdk.org/jdk/pull/23739
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v5]
On Tue, 4 Mar 2025 08:22:03 GMT, Thomas Schatzl wrote: >> src/hotspot/share/gc/g1/g1ConcurrentRefineStats.hpp line 43: >> >>> 41: size_t _cards_clean;// Number of cards found clean. >>> 42: size_t _cards_not_parsable; // Number of cards we could not >>> parse and left unrefined. >>> 43: size_t _cards_still_refer_to_cset; // Number of cards marked still >>> young. >> >> `_cards_still_refer_to_cset` from the naming it is not clear what the >> difference is with `_cards_refer_to_cset`, the comment is not helping with >> that > > `cards_still_refer_to_cset` refers to cards that were found to have already > been marked as `to-collection-set`. Renamed to > `_cards_already_refer_to_cset`, would that be okay? Fwiw, this is just for statistics, so if you want I can remove these. I did some experiments with re-examining these cards too to see whether we could clear them later. For determining if/when to do that a rate of increase for the young cards has been interesting. As mentioned, if you want I can remove them. - PR Review Comment: https://git.openjdk.org/jdk/pull/23739#discussion_r1978896272
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v7]
> Hi all, > > please review this change that implements (currently Draft) JEP: G1: > Improve Application Throughput with a More Efficient Write-Barrier. > > The reason for posting this early is that this is a large change, and the JEP > process is already taking very long with no end in sight but we would like to > have this ready by JDK 25. > > ### Current situation > > With this change, G1 will reduce the post write barrier to much more resemble > Parallel GC's as described in the JEP. The reason is that G1 lacks in > throughput compared to Parallel/Serial GC due to larger barrier. > > The main reason for the current barrier is how g1 implements concurrent > refinement: > * g1 tracks dirtied cards using sets (dirty card queue set - dcqs) of buffers > (dirty card queues - dcq) containing the location of dirtied cards. > Refinement threads pick up their contents to re-refine. The barrier needs to > enqueue card locations. > * For correctness dirty card updates requires fine-grained synchronization > between mutator and refinement threads, > * Finally there is generic code to avoid dirtying cards altogether (filters), > to avoid executing the synchronization and the enqueuing as much as possible. > > These tasks require the current barrier to look as follows for an assignment > `x.a = y` in pseudo code: > > > // Filtering > if (region(@x.a) == region(y)) goto done; // same region check > if (y == null) goto done; // null value check > if (card(@x.a) == young_card) goto done; // write to young gen check > StoreLoad;// synchronize > if (card(@x.a) == dirty_card) goto done; > > *card(@x.a) = dirty > > // Card tracking > enqueue(card-address(@x.a)) into thread-local-dcq; > if (thread-local-dcq is not full) goto done; > > call runtime to move thread-local-dcq into dcqs > > done: > > > Overall this post-write barrier alone is in the range of 40-50 total > instructions, compared to three or four(!) for parallel and serial gc. > > The large size of the inlined barrier not only has a large code footprint, > but also prevents some compiler optimizations like loop unrolling or inlining. > > There are several papers showing that this barrier alone can decrease > throughput by 10-20% > ([Yang12](https://dl.acm.org/doi/10.1145/2426642.2259004)), which is > corroborated by some benchmarks (see links). > > The main idea for this change is to not use fine-grained synchronization > between refinement and mutator threads, but coarse grained based on > atomically switching card tables. Mutators only work on the "primary" card > table, refinement threads on a se... Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: * iwalulya initial comments * renaming * made blend() helper function more clear; at least gcc will optimize it to the same code as before - Changes: - all: https://git.openjdk.org/jdk/pull/23739/files - new: https://git.openjdk.org/jdk/pull/23739/files/b3dd0084..8f46dc9a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23739&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23739&range=05-06 Stats: 27 lines in 9 files changed: 7 ins; 3 del; 17 mod Patch: https://git.openjdk.org/jdk/pull/23739.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23739/head:pull/23739 PR: https://git.openjdk.org/jdk/pull/23739
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v10]
> Hi all, > > please review this change that implements (currently Draft) JEP: G1: > Improve Application Throughput with a More Efficient Write-Barrier. > > The reason for posting this early is that this is a large change, and the JEP > process is already taking very long with no end in sight but we would like to > have this ready by JDK 25. > > ### Current situation > > With this change, G1 will reduce the post write barrier to much more resemble > Parallel GC's as described in the JEP. The reason is that G1 lacks in > throughput compared to Parallel/Serial GC due to larger barrier. > > The main reason for the current barrier is how g1 implements concurrent > refinement: > * g1 tracks dirtied cards using sets (dirty card queue set - dcqs) of buffers > (dirty card queues - dcq) containing the location of dirtied cards. > Refinement threads pick up their contents to re-refine. The barrier needs to > enqueue card locations. > * For correctness dirty card updates requires fine-grained synchronization > between mutator and refinement threads, > * Finally there is generic code to avoid dirtying cards altogether (filters), > to avoid executing the synchronization and the enqueuing as much as possible. > > These tasks require the current barrier to look as follows for an assignment > `x.a = y` in pseudo code: > > > // Filtering > if (region(@x.a) == region(y)) goto done; // same region check > if (y == null) goto done; // null value check > if (card(@x.a) == young_card) goto done; // write to young gen check > StoreLoad;// synchronize > if (card(@x.a) == dirty_card) goto done; > > *card(@x.a) = dirty > > // Card tracking > enqueue(card-address(@x.a)) into thread-local-dcq; > if (thread-local-dcq is not full) goto done; > > call runtime to move thread-local-dcq into dcqs > > done: > > > Overall this post-write barrier alone is in the range of 40-50 total > instructions, compared to three or four(!) for parallel and serial gc. > > The large size of the inlined barrier not only has a large code footprint, > but also prevents some compiler optimizations like loop unrolling or inlining. > > There are several papers showing that this barrier alone can decrease > throughput by 10-20% > ([Yang12](https://dl.acm.org/doi/10.1145/2426642.2259004)), which is > corroborated by some benchmarks (see links). > > The main idea for this change is to not use fine-grained synchronization > between refinement and mutator threads, but coarse grained based on > atomically switching card tables. Mutators only work on the "primary" card > table, refinement threads on a se... Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision: * ayang review - fix comment - Changes: - all: https://git.openjdk.org/jdk/pull/23739/files - new: https://git.openjdk.org/jdk/pull/23739/files/442d9eae..fc674f02 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23739&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23739&range=08-09 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/23739.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23739/head:pull/23739 PR: https://git.openjdk.org/jdk/pull/23739
Re: RFR: 8350811: [JMH] test foreign.StrLenTest failed with StringIndexOutOfBoundsException for size=451
On Mon, 3 Mar 2025 20:24:54 GMT, Vladimir Ivanov wrote:
> test setup was updated to generate data of requested size.
Looks good. Just 1 minor question
test/micro/org/openjdk/bench/java/lang/foreign/ToCStringTest.java line 100:
> 98: mollit anim id est laborum.
> 99: """;
> 100: while (lorem.length() < size) lorem += lorem;
Minor: just out of interest, is there any particular reason you're using
```java
while (lorem.length() < size) lorem += lorem;
```
instead of
```java
while (lorem.length() < size) {
lorem += lorem;
}
?
It seems to me it might be harder to read, but it's fine as it is too
-
PR Review: https://git.openjdk.org/jdk/pull/23873#pullrequestreview-2657018870
PR Review Comment: https://git.openjdk.org/jdk/pull/23873#discussion_r1979206364
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v9]
On Tue, 4 Mar 2025 09:57:56 GMT, Thomas Schatzl wrote:
>> Hi all,
>>
>> please review this change that implements (currently Draft) JEP: G1:
>> Improve Application Throughput with a More Efficient Write-Barrier.
>>
>> The reason for posting this early is that this is a large change, and the
>> JEP process is already taking very long with no end in sight but we would
>> like to have this ready by JDK 25.
>>
>> ### Current situation
>>
>> With this change, G1 will reduce the post write barrier to much more
>> resemble Parallel GC's as described in the JEP. The reason is that G1 lacks
>> in throughput compared to Parallel/Serial GC due to larger barrier.
>>
>> The main reason for the current barrier is how g1 implements concurrent
>> refinement:
>> * g1 tracks dirtied cards using sets (dirty card queue set - dcqs) of
>> buffers (dirty card queues - dcq) containing the location of dirtied cards.
>> Refinement threads pick up their contents to re-refine. The barrier needs to
>> enqueue card locations.
>> * For correctness dirty card updates requires fine-grained synchronization
>> between mutator and refinement threads,
>> * Finally there is generic code to avoid dirtying cards altogether
>> (filters), to avoid executing the synchronization and the enqueuing as much
>> as possible.
>>
>> These tasks require the current barrier to look as follows for an assignment
>> `x.a = y` in pseudo code:
>>
>>
>> // Filtering
>> if (region(@x.a) == region(y)) goto done; // same region check
>> if (y == null) goto done; // null value check
>> if (card(@x.a) == young_card) goto done; // write to young gen check
>> StoreLoad;// synchronize
>> if (card(@x.a) == dirty_card) goto done;
>>
>> *card(@x.a) = dirty
>>
>> // Card tracking
>> enqueue(card-address(@x.a)) into thread-local-dcq;
>> if (thread-local-dcq is not full) goto done;
>>
>> call runtime to move thread-local-dcq into dcqs
>>
>> done:
>>
>>
>> Overall this post-write barrier alone is in the range of 40-50 total
>> instructions, compared to three or four(!) for parallel and serial gc.
>>
>> The large size of the inlined barrier not only has a large code footprint,
>> but also prevents some compiler optimizations like loop unrolling or
>> inlining.
>>
>> There are several papers showing that this barrier alone can decrease
>> throughput by 10-20%
>> ([Yang12](https://dl.acm.org/doi/10.1145/2426642.2259004)), which is
>> corroborated by some benchmarks (see links).
>>
>> The main idea for this change is to not use fine-grained synchronization
>> between refinement and mutator threads, but coarse grained based on
>> atomically switching c...
>
> Thomas Schatzl has updated the pull request incrementally with one additional
> commit since the last revision:
>
> * iwalulya review 2
> * G1ConcurrentRefineWorkState -> G1ConcurrentRefineSweepState
> * some additional documentation
src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp line 108:
> 106:
> 107: void G1ConcurrentRefineThreadControl::control_thread_do(ThreadClosure*
> tc) {
> 108: if (_control_thread != nullptr) {
maybe maintain using `if (max_num_threads() > 0)` as used in
`G1ConcurrentRefineThreadControl::initialize`, so that it is clear that setting
`G1ConcRefinementThreads=0` effectively turns off concurrent refinement.
src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp line 354:
> 352: if (!r->is_free()) {
> 353: // Need to scan all parts of non-free regions, so reset the
> claim.
> 354: // No need for synchronization: we are only interested about
> regions
s/about/in
src/hotspot/share/gc/g1/g1OopClosures.hpp line 205:
> 203: G1CollectedHeap* _g1h;
> 204: uint _worker_id;
> 205: bool _has_to_cset_ref;
Similar to `_cards_refer_to_cset` , do you mind renaming `_has_to_cset_ref`
and `_has_to_old_ref` to `_has_ref_to_cset` and `_has_ref_to_old`
src/hotspot/share/gc/g1/g1Policy.hpp line 105:
> 103: uint _free_regions_at_end_of_collection;
> 104:
> 105: size_t _pending_cards_from_gc;
A comment on the variable would be nice, especially on how it is set/reset both
at end of GC and by refinement. Also the `_to_collection_set_cards` below could
use a comment
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23739#discussion_r1979077904
PR Review Comment: https://git.openjdk.org/jdk/pull/23739#discussion_r1979102189
PR Review Comment: https://git.openjdk.org/jdk/pull/23739#discussion_r1979212854
PR Review Comment: https://git.openjdk.org/jdk/pull/23739#discussion_r1979155941
Re: RFR: 8343191: Cgroup v1 subsystem fails to set subsystem path [v17]
On Fri, 28 Feb 2025 20:40:37 GMT, Sergey Chernyshev wrote: >> Cgroup V1 subsustem fails to initialize mounted controllers properly in >> certain cases, that may lead to controllers left undetected/inactive. We >> observed the behavior in CloudFoundry deployments, it affects also host >> systems. >> >> The relevant /proc/self/mountinfo line is >> >> >> 2207 2196 0:43 >> /system.slice/garden.service/garden/good/2f57368b-0eda-4e52-64d8-af5c >> /sys/fs/cgroup/cpu,cpuacct ro,nosuid,nodev,noexec,relatime master:25 - >> cgroup cgroup rw,cpu,cpuacct >> >> >> /proc/self/cgroup: >> >> >> 11:cpu,cpuacct:/system.slice/garden.service/garden/bad/2f57368b-0eda-4e52-64d8-af5c >> >> >> Here, Java runs inside containerized process that is being moved cgroups due >> to load balancing. >> >> Let's examine the condition at line 64 here >> https://github.com/openjdk/jdk/blob/55a7cf14453b6cd1de91362927b2fa63cba400a1/src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp#L59-L72 >> It is always FALSE and the branch is never taken. The issue was spotted >> earlier by @jerboaa in >> [JDK-8288019](https://bugs.openjdk.org/browse/JDK-8288019). >> >> The original logic was intended to find the common prefix of `_root`and >> `cgroup_path` and concatenate the remaining suffix to the `_mount_point` >> (lines 67-68). That could lead to the following results: >> >> Example input >> >> _root = "/a" >> cgroup_path = "/a/b" >> _mount_point = "/sys/fs/cgroup/cpu,cpuacct" >> >> >> result _path >> >> "/sys/fs/cgroup/cpu,cpuacct/b" >> >> >> Here, cgroup_path comes from /proc/self/cgroup 3rd column. The man page >> (https://man7.org/linux/man-pages/man7/cgroups.7.html#NOTES) for control >> groups states: >> >> >> ... >>/proc/pid/cgroup (since Linux 2.6.24) >> This file describes control groups to which the process >> with the corresponding PID belongs. The displayed >> information differs for cgroups version 1 and version 2 >> hierarchies. >> For each cgroup hierarchy of which the process is a >> member, there is one entry containing three colon- >> separated fields: >> >> hierarchy-ID:controller-list:cgroup-path >> >> For example: >> >> 5:cpuacct,cpu,cpuset:/daemons >> ... >> [3] This field contains the pathname of the control group >>in the hierarchy to which the process belongs. This >>pathname is relative to the mount point of the >>hierarchy. >> >> >> This explicitly states the "pathname is relative t... > > Sergey Chernyshev has updated the pull request incrementally with one > additional commit since the last revision: > > updated copyright headers @sercher Your change (at version bae78ff7c43e0446ef583db1c43b4a3d6c06ad4f) is now ready to be sponsored by a Committer. - PR Comment: https://git.openjdk.org/jdk/pull/21808#issuecomment-2697178573
RFR: 8344708: Compiler Implementation of Module Import Declarations
This is a patch to finalize the module imports feature. Please see: https://bugs.openjdk.org/browse/JDK-8344700 - Commit messages: - Cleanup. - Cleanup. - Finalizing module imports. Changes: https://git.openjdk.org/jdk/pull/23801/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=23801&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8344708 Stats: 113 lines in 15 files changed: 9 ins; 71 del; 33 mod Patch: https://git.openjdk.org/jdk/pull/23801.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23801/head:pull/23801 PR: https://git.openjdk.org/jdk/pull/23801
Re: RFR: 8350685: java/lang/System/SecurityManagerWarnings.java fails with --enable-preview
On Tue, 4 Mar 2025 10:15:18 GMT, Jaikiran Pai wrote: > I believe the proposed change here to package the necessary class files in > the JAR file, so that they are available when launching java with that JAR > file in the classpath, is correct. I just realized one thing - this `OutputAnalyzer` is a test library class. So `test.classes` directory won't always contain this library class (jtreg sometimes compiles it into a separate directory and that behaviour is non-determinstic). So you might have to include additional paths when constructing that JAR file. - PR Comment: https://git.openjdk.org/jdk/pull/23864#issuecomment-2697219880
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v5]
On Mon, 3 Mar 2025 18:50:37 GMT, Ivan Walulya wrote:
>> Thomas Schatzl has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> * fix comment (trailing whitespace)
>> * another assert when snapshotting at a safepoint.
>
> src/hotspot/share/gc/g1/g1ConcurrentRefine.hpp line 84:
>
>> 82: // Tracks the current refinement state from idle to completion (and
>> reset back
>> 83: // to idle).
>> 84: class G1ConcurrentRefineWorkState {
>
> G1ConcurrentRefinementState? I am not convinced the "Work" adds any clarity
We agreed on `G1ConcurrentRefineSweepState` for now, better suggestions welcome.
Use `Refine` instead of `Refinement` since all pre-existing classes also use
`Refine`. This could be renamed in an extra change.
> src/hotspot/share/gc/g1/g1ConcurrentRefine.hpp line 113:
>
>> 111: // Current epoch the work has been started; used to determine if
>> there has been
>> 112: // a forced card table swap due to a garbage collection while doing
>> work.
>> 113: size_t _refine_work_epoch;
>
> same as previous comment, why `refine_work` instead of `refinement`?
Already renamed, same as previous comment.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23739#discussion_r1979050867
PR Review Comment: https://git.openjdk.org/jdk/pull/23739#discussion_r1979051649
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v5]
On Tue, 4 Mar 2025 09:52:40 GMT, Thomas Schatzl wrote:
>> src/hotspot/share/gc/g1/g1ConcurrentRefine.hpp line 84:
>>
>>> 82: // Tracks the current refinement state from idle to completion (and
>>> reset back
>>> 83: // to idle).
>>> 84: class G1ConcurrentRefineWorkState {
>>
>> G1ConcurrentRefinementState? I am not convinced the "Work" adds any clarity
>
> We agreed on `G1ConcurrentRefineSweepState` for now, better suggestions
> welcome.
>
> Use `Refine` instead of `Refinement` since all pre-existing classes also use
> `Refine`. This could be renamed in an extra change.
Add the `Sweep` in the name because this is not the state for entire refinement
(which also includes information about when to start refinement/sweeping).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23739#discussion_r1979053344
Re: RFR: 8350685: java/lang/System/SecurityManagerWarnings.java fails with --enable-preview
On Mon, 3 Mar 2025 13:33:05 GMT, Sean Mullan wrote: > Please review this fix to a test that fails when run with --enable-preview. > This fix is to add all the necessary utility classes imported by the test to > the JAR file, and not just the test classes. The description in the JBS issue wasn't clear why this test would fail in this manner with `--enable-preview`. I looked at this a bit and I've added a comment to that JBS issue explaining what the issue is about. I believe the proposed change here to package the necessary class files in the JAR file, so that they are available when launching `java` with that JAR file in the classpath, is correct. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23864#pullrequestreview-2656806335
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v11]
On Tue, 4 Mar 2025 15:56:05 GMT, Thomas Schatzl wrote: > It's fast anyway. To clarify: If you have multiple refinement rounds between two garbage collections, the time to clear the young gen cards is almost noise compared to the actual refinement effort. Like two magnitudes faster. - PR Review Comment: https://git.openjdk.org/jdk/pull/23739#discussion_r1979785011
Re: RFR: 8342382: Implementation of JEP G1: Improve Application Throughput with a More Efficient Write-Barrier [v11]
On Tue, 4 Mar 2025 16:04:00 GMT, Thomas Schatzl wrote: >> src/hotspot/share/gc/g1/g1ConcurrentRefineThread.cpp line 219: >> >>> 217: // The young gen revising mechanism reads the predictor and the >>> values set >>> 218: // here. Avoid inconsistencies by locking. >>> 219: MutexLocker x(G1RareEvent_lock, Mutex::_no_safepoint_check_flag); >> >> Who else can be in this critical-section? I don't get what this lock is >> protecting us from. > > The concurrent refine control thread in > `G1ConcurrentRefineThread::do_refinement`, when calling > `G1Policy::record_dirtying_stats`. I could create an extra mutex for that if you want to make it clear which two parties access the same data. - PR Review Comment: https://git.openjdk.org/jdk/pull/23739#discussion_r1979810144
Integrated: 8350013: Add a test for JDK-8150442
On Thu, 27 Feb 2025 14:17:30 GMT, Alexey Semenyuk wrote: > Add a test to verify jpackage is using a custom MSI condition blocking > package installation depending on the version of Windows where the package > installer runs. Support for this MSI condition was added in > [JDK-8150442](https://bugs.openjdk.org/browse/JDK-8150442). > > The test adds an unconditionally failing OS-version MSI condition to the > resource directory. MSI installer using this condition should always fail. > The exit code of the failed installation would be 1603. Extended error > information can be dug in the MSI log file. To make the test work, the > following changes to jpackage test lib have been made: > - Support non-0 exit code from MSI install handler. Support for non-0 exit > codes was added to install handlers of all other types too. Added > `PackageTest.setExpectedInstallExitCode(int)` method to configure the > expected exit code of a package installation; > - Support using msi log files when MSI and EXE packages get installed, > unpacked, or uninstalled. Added `PackageTest.createMsiLog(boolean)` to enable > or disable creation of msi log files in a PackageTest instance and > `Optional JPackageCommand.winMsiLogFile()` to access the current log > file from the callbacks registered with the PackageTest instance. > > Added tests for PackageTest class (PackageTestTest). > > Additionally, improved the code in WindowsHelper detecting paths to Start > Menu, Desktop, and other common paths. Previously, it relied on reading these > paths from the registry. On some machines, required registry keys are not > available. The code now will call .NET `Environment.GetFolderPath()` function > through powershell if a registry key is missing. This pull request has now been integrated. Changeset: 3e86b3a8 Author:Alexey Semenyuk URL: https://git.openjdk.org/jdk/commit/3e86b3a879c7a425e7c689142cb1f0fdd4f679ed Stats: 1898 lines in 8 files changed: 1419 ins; 181 del; 298 mod 8350013: Add a test for JDK-8150442 Reviewed-by: almatvee - PR: https://git.openjdk.org/jdk/pull/23825
Re: RFR: 8350607: Consolidate MethodHandles::zero into MethodHandles::constant [v2]
On Mon, 24 Feb 2025 23:45:37 GMT, Chen Liang wrote: >> LF editor spins classes, this avoids the spinning overhead and should speed >> up non-capturing lambdas too. >> >> There may need to be additional intrinsic work for MH combinator lf bytecode >> generation. > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > We no longer load DelegateMH as we no longer rebind I hope that the intrinsic mechanism can be further simplified, at some point. But I defer to Jorn's comments about tableswitch. Excellent cleanups. Lots and lots of deleted code, and other code regularized. Thank you! src/java.base/share/classes/java/lang/invoke/SimpleMethodHandle.java line 36: > 34: * A method handle whose behavior is determined only by its LambdaForm. > 35: * Access to SimpleMethodHandle should ensure BoundMethodHandle is > initialized > 36: * first. Did you try factoring UNSAFE.ensureInit(BMH) into a static block in SimpleMethodHandle.java? Sometimes that works. - Marked as reviewed by jrose (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23706#pullrequestreview-2658926068 PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1980190604
Re: RFR: 8351045: ClassValue::remove cannot ensure computation observes up-to-date state
On Mon, 3 Mar 2025 15:24:05 GMT, Chen Liang wrote: > After a call to `ClassValue.remove`, a `ClassValue` can still install a value > that is computed with information that is not up-to-date with the remove > call. This is demonstrated in the test case, where an innocuous > `ClassValue.get` call on an uncomputed CV may happen to compute during which > a remove happened, and proceed to install an outdated value onto a CV. > > The fix is simple - to force computation to retry when a remove has happened, > so the retry can observe the up-to-date states from the remove. (finishEntry > and removeEntry are both synchronized on the object monitor of the > ClassValueMap instance) Test is good also. src/java.base/share/classes/java/lang/ClassValue.java line 504: > 502: Entry e = remove(classValue.identity); > 503: // e == null: Uninitialized, and no pending calls to > computeValue. > 504: // remove didn't change anything. No change. Please capitalize the sentence: "Remove didn't…" Also "Remove and discard" instead of "remove discarded". Perhaps (this is being very picky) "Inside finishEntry, the logic will retry when it discovers the promise is removed." This is a good bug fix. Thanks for finding it. - Marked as reviewed by jrose (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23866#pullrequestreview-2658737905 PR Review Comment: https://git.openjdk.org/jdk/pull/23866#discussion_r1980091146
Re: RFR: 8344708: Compiler Implementation of Module Import Declarations
On Wed, 26 Feb 2025 13:50:52 GMT, Jan Lahoda wrote: > This is a patch to finalize the module imports feature. Please see: > https://bugs.openjdk.org/browse/JDK-8344700 looks sensible test/langtools/tools/javac/modules/EdgeCases.java line 1190: > 1188: > 1189: List expected = List.of( > 1190: "Test.java:2:8: > compiler.err.feature.not.supported.in.source.plural: > (compiler.misc.feature.module.imports), 24, 25", isn't there a similar test above? - Marked as reviewed by vromero (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23801#pullrequestreview-2658747032 PR Review Comment: https://git.openjdk.org/jdk/pull/23801#discussion_r1980096065
Re: RFR: 8350685: java/lang/System/SecurityManagerWarnings.java fails with --enable-preview
On Mon, 3 Mar 2025 13:33:05 GMT, Sean Mullan wrote: > Please review this fix to a test that fails when run with --enable-preview. > This fix is to add all the necessary utility classes imported by the test to > the JAR file, and not just the test classes. I am withdrawing this PR for now and marking the bug as a duplicate of [JDK-8351188](https://bugs.openjdk.org/browse/JDK-8351188). Although we could workaround the issue in the test, it is not causing normal mach 5 tests to fail (since they don't run with --enable-preview), so I think it is best to wait and see how this issue is resolved first. - PR Comment: https://git.openjdk.org/jdk/pull/23864#issuecomment-2698690691
Withdrawn: 8350685: java/lang/System/SecurityManagerWarnings.java fails with --enable-preview
On Mon, 3 Mar 2025 13:33:05 GMT, Sean Mullan wrote: > Please review this fix to a test that fails when run with --enable-preview. > This fix is to add all the necessary utility classes imported by the test to > the JAR file, and not just the test classes. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/23864
Re: RFR: 8350811: [JMH] test foreign.StrLenTest failed with StringIndexOutOfBoundsException for size=451 [v2]
> test setup was updated to generate data of requested size. Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision: JDK-8350811 [JMH] test foreign.StrLenTest failed with StringIndexOutOfBoundsException for size=451 - Changes: - all: https://git.openjdk.org/jdk/pull/23873/files - new: https://git.openjdk.org/jdk/pull/23873/files/e5b2b4b0..69207701 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23873&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23873&range=00-01 Stats: 8 lines in 3 files changed: 5 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/23873.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23873/head:pull/23873 PR: https://git.openjdk.org/jdk/pull/23873
Re: RFR: 8350811: [JMH] test foreign.StrLenTest failed with StringIndexOutOfBoundsException for size=451
On Mon, 3 Mar 2025 20:24:54 GMT, Vladimir Ivanov wrote: > test setup was updated to generate data of requested size. Update to regular cycle template. Thanks - PR Comment: https://git.openjdk.org/jdk/pull/23873#issuecomment-2698720228
Re: RFR: 8266431: Dual-Pivot Quicksort improvements (Radix sort) [v11]
On Sun, 22 Oct 2023 17:26:52 GMT, Laurent Bourgès wrote: >> * improved mixed insertion sort (makes whole sorting faster) >> * introduced Radix which sort shows several times boost of performance and >> has linear complexity instead of n*ln(n) >> * improved merging sort for almost sorted data >> * optimized parallel sorting >> * improved step for pivot candidates and pivot partitioning >> * extended existing tests >> * added benchmarking JMH tests >> * suggested better buffer allocation: if no memory, it is switched to >> in-place sorting with no OutOfMemoryError, threshold is 1/16th heap >> >> I am going on previous PR by Vladimir Yaroslavskyi: >> https://github.com/openjdk/jdk/pull/3938 > > Laurent Bourgès has updated the pull request incrementally with one > additional commit since the last revision: > > add @SuppressWarnings (serial) Sorting of short/byte/char is almost finished - PR Comment: https://git.openjdk.org/jdk/pull/13568#issuecomment-2698726394
Re: RFR: 8350685: java/lang/System/SecurityManagerWarnings.java fails with --enable-preview
On Mon, 3 Mar 2025 13:33:05 GMT, Sean Mullan wrote: > Please review this fix to a test that fails when run with --enable-preview. > This fix is to add all the necessary utility classes imported by the test to > the JAR file, and not just the test classes. The core reflection used by the launcher can provoke linkage errors. Same thing happens in other usages when looking for constructors or methods. That said, it does appear that this test creates a JAR file containing classes with dangling references. Changing the change to launch a helper class rather than launching SecurityManagerWarnings, and adding the helper to the JAR file, would avoid this. - PR Comment: https://git.openjdk.org/jdk/pull/23864#issuecomment-2698727500
Re: [External] : Re: RFR: 8350594: Misleading warning about install dir for DMG packaging
> On Mar 4, 2025, at 10:20 AM, Alexey Semenyuk > wrote: > > > > On 3/4/2025 11:06 AM, Michael Hall wrote: >>> Good point about `CFBundleGetInfoString`. Maybe jpackage should set this >>> field with the value of `--description` option. Another finding! >>> >>> As for editing "Info.plist" from the runtime source, if the directory >>> referenced from `--runtime-image` option is a valid bundle, i.e. has >>> "Contents/MacOS/libjli.dylib", "Contents/Info.plist" files, and >>> "Contents/_CodeSignature" elements, and there are no other options, like >>> `--app-version`, jpackage should use the supplied directory as-is without >>> editing the existing "Info.plist" file. Otherwise, jpackage should create >>> "Info.plist" and make sure the values are consistent, of course. >> I posted an example, there are a few other places in Info.plist that would >> also need changing to the correct version if you don’t already have a valid >> Info.plist for the JRE. The page of mine that you linked I think mentioned >> them all. >> >> __CodeSignature I didn’t include in what I did. It seemed like you might >> not be able to copy this but would need to do your own signing to generate >> it for a different JRE? My guess was that it is only really needed for an >> application distribution including that JRE. I know jpackage does some of >> its own signing. If this recreates the JRE __CodeSignature, this probably >> wouldn’t be a concern. >> >> It is possible if you are provided with a JRE where all this is correct then >> copying as-is would be fine. > Right. If you package unpacked official OpenJDK tar.gz jpackage should take > it as is. > >> However, if you are copying a JRE that already has a JavaVirtualMachines >> file for that version I’m not sure what would happen. > The same as in the above case, I guess. jpackage should take it as is. > >> That should be checked? If you are providing a later release the new one >> should be used. > Sorry, I don't follow. What release do you mean? > >> If you are providing an earlier release the one you are adding will be >> ignored. Warnings at least? /usr/libexec/java_home -v could >> maybe be useful here. I think though you would need to determine the version >> of what you are copying in. >> >> Assuming the JRE you are adding to JavaVirtualMachines is a unique or >> current one for that java version then I think some fairly simple test like >> java —version or something again with /usr/libexec/java_home could verify >> that everything is alright. > jpackage doesn't control how you use bundles it produces. You can use the > same input runtime image and create two separate bundles with different > names, but with the same versions and install them both. > > - Alexey Sorry again that I was thinking something could be done for the actual install on the users machine which it can’t. If you are using this for official release installs yes, it should be no problem copying it as-is. More or less what I do manually and it has worked just fine. If it is a jpackage option that others might use, which as you indicate, it is. My suggestion is that jpackage can do some checking of its validity. I was trying a number of different things, some very different and all were accepted, none were valid JavaVirtualMachine system runtimes. As a further enhancement I think it would be possible if the MacOS needed files are not there for jpackage to supply them to make it a valid JavaVirtualMachines system jre. My Info.plist example showed a way you could make all needed version/description changes using /usr/libexec/PlistBuddy. I believe jpackage currently relies on issuing just such native commands. Maybe some future use of that for jpackage could also turn up. This is why I thought about writing a plist editor for jpackage application build use. Signing if needed I’m sure you could manage, you probably already address the issues. As mentioned I manually put together a JavaVirtualMachines runtime from a built jdk that seemed to work fine command line and when used as the user specified runtime for a fairly complex application. For some reason it didn’t work to automatically jlink from. This led to other issues when I tried to address —add-modules needed for JavaFX. So there might be some non-trivial complications that I didn’t resolve. Thanks yet again for the follow-up.
