Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17389 )
Change subject: IMPALA-10680: Replace StringToFloatInternal using fast_double_parser library ...................................................................... Patch Set 6: (2 comments) Thanks for the changes! http://gerrit.cloudera.org:8080/#/c/17389/6/be/src/util/string-parser.h File be/src/util/string-parser.h: http://gerrit.cloudera.org:8080/#/c/17389/6/be/src/util/string-parser.h@545 PS6, Line 545: std::string res; I wonder if the performance numbers are still the same, given that now we need to dynamically allocate memory from the heap. (Though for short strings the standard library implementation being used avoids allocation by using Small String Optimization) http://gerrit.cloudera.org:8080/#/c/17389/6/be/src/util/string-parser.h@552 PS6, Line 552: // string has `move` defined, which would be used instead of copy. nit: I don't think we need this comment. And I think the situation is even better as in this case Named Return Value Optimization kicks in which even avoids moving: https://en.cppreference.com/w/cpp/language/copy_elision -- To view, visit http://gerrit.cloudera.org:8080/17389 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic105ad38a2fcbf2fb4e8ae8af6d9a8e251a9c141 Gerrit-Change-Number: 17389 Gerrit-PatchSet: 6 Gerrit-Owner: Amogh Margoor <[email protected]> Gerrit-Reviewer: Amogh Margoor <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 21 May 2021 09:29:12 +0000 Gerrit-HasComments: Yes
