Zoltan Martonka has posted comments on this change. ( http://gerrit.cloudera.org:8080/21127 )
Change subject: [ARM] Concurrent binary tree memory barriers fixed. ...................................................................... Patch Set 9: > (1 comment) > > > (1 comment) > > > > I realized that, the only proper way to add a 2-way load barrier > is > > with > > std::atomic_thread_fence(std::memory_order_acquire), which is > more > > strict, than > > the 1-way std::atomic<T>::load(std::memory_order_acquire); > > > > I made some performance test. I used TestScanPerformance from > > cbtree-test and > > TestMemRowSetUpdatePerformance from memrowset-test. I also added > 2 > > more > > performance tests, one using basic arena, one using > > ThreadSafeMemoryTrackingArena (we only use the 2nd in > production). > > I will add g++ tests too (as soon as it compiles). > > See the branches sheet for name resolution. > > For some reason, I assumed you were going to add those tests into > this patch or as a separate patch, but it seems by 'adding > > > https://docs.google.com/spreadsheets/d/1whl8h7S0DcjVYp0jksR87P9v4U8TQH9nS1C9N0D-lUo/edit?usp=sharing > > > (1 comment) > > > > I realized that, the only proper way to add a 2-way load barrier > is > > with > > std::atomic_thread_fence(std::memory_order_acquire), which is > more > > strict, than > > the 1-way std::atomic<T>::load(std::memory_order_acquire); > > > > I made some performance test. I used TestScanPerformance from > > cbtree-test and > > TestMemRowSetUpdatePerformance from memrowset-test. I also added > 2 > > more > > performance tests, one using basic arena, one using > > ThreadSafeMemoryTrackingArena (we only use the 2nd in > production). > > I will add g++ tests too (as soon as it compiles). > > See the branches sheet for name resolution. > > For some reason, I assumed you were going to add your new tests as > a part of this changelist as well, but it seems you just meant > adding results from running those tests into the spreadsheet :) > > OK, maybe it makes sense to add your new tests into Kudu as well? > Feel free to post that as a separate changelist. > > Also, it would be great if you could address other feedback items > (nits, questions of other reviewers, etc.) and post a follow-up > revision, if necessary. > > Thanks! > > > https://docs.google.com/spreadsheets/d/1whl8h7S0DcjVYp0jksR87P9v4U8TQH9nS1C9N0D-lUo/edit?usp=sharing I run the arm tests on a c6g.2xlarge arm machine, and the x86 tests on a physical pc in the bp office (I can include the spec if you want). I thought only the changes matter (not the comparison of x86 and ARM), however I can try to rerun the tests on a similar x86 than the arm machine (c6i.2xlarge?)/ All times are in ms, the value that the google test prints (TestMemRowSetUpdatePerformance included). The (same platform) tests were run on the same machine. One at a time through ssh. No other task was taking place (I even closed VS Code to prevent it from doing any background task). x86_master_clang_2 and x86_master_clang are the same, only 3 hours apart (And I have no idea why the second run is a bit slower). My new tests are nearly the same as "TestConcurrentInsert", just using the ThreadSafeMemoryTrackingArena and the sizes that production actually uses. I could add a test with less inserts using this parameters. (If the tree works while it is small, it will work, when it is big, because threads will "collide" less often). I will post a separate gerrit for this test (Same as TestPerformanceThreadSafe, just runs <1s). -- To view, visit http://gerrit.cloudera.org:8080/21127 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie7c02dc3b9444599ef680fa9b29ae7a4d39dd382 Gerrit-Change-Number: 21127 Gerrit-PatchSet: 9 Gerrit-Owner: Zoltan Martonka <zmarto...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Anonymous Coward (763) Gerrit-Reviewer: Ashwani Raina <ara...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Chovan <zcho...@cloudera.com> Gerrit-Reviewer: Zoltan Martonka <zmarto...@cloudera.com> Gerrit-Comment-Date: Tue, 07 May 2024 13:14:22 +0000 Gerrit-HasComments: No
