RFR: Cleanup NumberSeq additions [v2]
Y. Srinivas Ramakrishna
ysr at openjdk.org
Thu Apr 27 23:59:23 UTC 2023
On Thu, 27 Apr 2023 20:20:53 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:
>> I have been looking into how to decrease our upstream exposure for `NumberSeq` changes, and it seems to be not solvable in an easy way. Normally, I would add `AbsSeq::add(AbsSeq& other)` methods and get to that from `HdrSeq::add(HdrSeq& other)`, and that almost works. The remaining bit is figuring out the decaying average/variance thingie.
>>
>> I spent an hour trying to come up with easily provable equations for _combining_ the decaying average/variance. The decaying average is somewhat easy, although there are boundary problems (decaying average treats 0-th element without adjustments, which requires remembering the `_last` element to compensate). Decaying variance looks remarkably scary in the few of my approaches.
>>
>> If we enter with current code upstream, it would be the uphill battle to implemented decays and prove they work. So instead, I took the alternative approach of doing everything inside Shenandoah code, except the decays. This reverts `numberSeq.*` to upstream state.
>>
>> Additional testing:
>> - [x] Linux x86_64 fastdebug `hotspot_gc_shenandoah`
>> - [x] Linux AArch64 fastdebug `hotspot_gc_shenandoah`
>> - [ ] GitFarm
>
> Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision:
>
> Clear the fields
LGTM, modulo @earthling-amzn 's suggestion and ideally extending the test to cover clear.
test/hotspot/gtest/gc/shenandoah/test_shenandoahNumberSeq.cpp line 112:
> 110: }
> 111:
> 112: TEST_VM_F(ShenandoahNumberSeqMergeTest, merge_test) {
You could add a `clear_test` to this, for completeness, checking that the fields are clear following the call. That was being checked at line 125 in the old test, but you could extend and check some of the other fields, viz, `_sum` etc.
-------------
Marked as reviewed by ysr (Author).
PR Review: https://git.openjdk.org/shenandoah/pull/268#pullrequestreview-1405047657
PR Review Comment: https://git.openjdk.org/shenandoah/pull/268#discussion_r1179801812
More information about the shenandoah-dev
mailing list