Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21127 )
Change subject: [ARM] Concurrent binary tree memory barriers fixed. ...................................................................... Patch Set 10: (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 http://gerrit.cloudera.org:8080/#/c/21127/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21127/6//COMMIT_MSG@33 PS6, Line 33: The following 6 barriers need to be more strict: > See the google sheet. Thanks for posting the numbers. A few questions: * What were the machines that x86_64 and ARM tests were run at? How close they are in terms of processing power (frequency, number of CPU cores)? * Do I understand correctly that both the x86_64 and ARM machines were the same for all the tests, just different binaries were run? * I'm not sure I understand what 'x86_master_clang_2' at the Summary tab is for? How is it different from 'x86_master_clang'? * The numbers posted at the Summary tab: are those seconds of test runtime or that's some completely different (e.g., number scanned/inserted keys), or for different tests they are of different units? * The numbers posted at the Summary tab for the 'TestMemRowSetUpdatePerformance' test: what are those? Are those just total runtime or some particular timing report part (inserting, counting, updating) of the test? Also, could you run existing corresponding Kudu tests (you can run you new tests as well, of course) prior and post your gerrit patch under the 'perf' utility (say, 10 runs) and report the results. No need to put them into a spreadsheet, just present the output from the 'perf stat' utility. You can browse 'git log' and search for the examples how running those tests is usually done. You might also be interested in perf-mem and may be in perf-lock, but reports just from generic 'perf' would be great already. Thanks a lot! https://man7.org/linux/man-pages/man1/perf.1.html -- 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: 10 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: Sat, 04 May 2024 03:06:59 +0000 Gerrit-HasComments: Yes
