Alexander Saydakov has posted comments on this change. ( http://gerrit.cloudera.org:8080/17869 )
Change subject: IMPALA-10956: datasketches UDFs: memory leak and merge overhead ...................................................................... Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/17869/4/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/17869/4/be/src/exprs/aggregate-functions-ir.cc@2075 PS4, Line 2075: agg_state_ptr->second = new (ctx->Allocate<datasketches::update_theta_sketch>()) : datasketches::update_theta_sketch( : datasketches::update_theta_sketch::builder().build()) > I looked through the patch again looking for potential leaks. bit I didn't Space is allocated just above, and placement new is called to put an object there. uninitilaized_fill_n does the same. I just did not quite like using it for one object. I think it is clearer to call placement new directly. http://gerrit.cloudera.org:8080/#/c/17869/4/be/src/exprs/aggregate-functions-ir.cc@2151 PS4, Line 2151: u.update(*dst_sketch_ptr); > Couldn't we use std::move here? update seems to be optimized for the rvalue Yes, we could since we discard this object later. I doubt it makes any difference in practice. Rvalue case is supported in the API, but I don't think it is handled differently in the Theta union. Anyway, this is not incorrect. In the worst case it is a missed slight optimization opportunity. -- To view, visit http://gerrit.cloudera.org:8080/17869 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8dd0e6736f4266f74f5f265f58d40a4e4707287f Gerrit-Change-Number: 17869 Gerrit-PatchSet: 4 Gerrit-Owner: Alexander Saydakov <[email protected]> Gerrit-Reviewer: Alexander Saydakov <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fucun Chu <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Fri, 10 Dec 2021 18:16:29 +0000 Gerrit-HasComments: Yes
