You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2017/01/11 19:55:08 UTC
[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient
Tim Armstrong has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5666
Change subject: IMPALA-4752: make ObjectPool more efficient
......................................................................
IMPALA-4752: make ObjectPool more efficient
Previously it was implemented as a vector of pointers to
dynamically allocated wrapper objects, where each wrapper object
stored a pointer to the object and a pointer to a vtable, which
had a pointer to a destructor, which then calls the actual
object's destructor.
Instead of doing this, this patch changes ObjectPool to stores
the object pointer and function pointer inline in the vector.
This avoids an unnecessary malloc() and free() pair per object.
Testing:
TODO
Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
---
M be/src/common/object-pool.h
1 file changed, 15 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/5666/1
--
To view, visit http://gerrit.cloudera.org:8080/5666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-4752: make ObjectPool more efficient
......................................................................
Patch Set 3: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5666/2/be/src/common/object-pool.h
File be/src/common/object-pool.h:
Line 43: objects_.emplace_back(
> I don't think it matters from a performance perspective - compilers are goo
sgtm
--
To view, visit http://gerrit.cloudera.org:8080/5666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-4752: make ObjectPool more efficient
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5666/2/be/src/common/object-pool.h
File be/src/common/object-pool.h:
Line 43: Element elem{t, [](void* obj){ delete reinterpret_cast<T*>(obj); }};
Will the compiler elide the extra copy of the push back here? Or would it make sense to construct elem in place using emplace_back?
--
To view, visit http://gerrit.cloudera.org:8080/5666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change.
Change subject: IMPALA-4752: make ObjectPool more efficient
......................................................................
Patch Set 3: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5666/3/be/src/common/object-pool.h
File be/src/common/object-pool.h:
PS3, Line 60: //
'///' ?
--
To view, visit http://gerrit.cloudera.org:8080/5666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4752: make ObjectPool more efficient
......................................................................
Patch Set 4:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/186/
--
To view, visit http://gerrit.cloudera.org:8080/5666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4752: make ObjectPool more efficient
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5666/2/be/src/common/object-pool.h
File be/src/common/object-pool.h:
Line 43: Element elem{t, [](void* obj){ delete reinterpret_cast<T*>(obj); }};
> Will the compiler elide the extra copy of the push back here? Or would it m
It should be able to optimise this fine, since it's a plain struct and it just needs to copy the two fields.
I wanted to use emplace_back() for brevity but I couldn't get it to work without defining an explicit constructor. Looks like i was missing the trick to get it to work with the braced initialisation syntax:
http://stackoverflow.com/questions/13812703/c11-emplace-back-on-vectorstruct
--
To view, visit http://gerrit.cloudera.org:8080/5666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has uploaded a new patch set (#3).
Change subject: IMPALA-4752: make ObjectPool more efficient
......................................................................
IMPALA-4752: make ObjectPool more efficient
Previously it was implemented as a vector of pointers to
dynamically allocated wrapper objects, where each wrapper object
stored a pointer to the object and a pointer to a vtable, which
had a pointer to a destructor, which then calls the actual
object's destructor.
Instead of doing this, this patch changes ObjectPool to stores
the object pointer and function pointer inline in the vector.
This avoids an unnecessary malloc() and free() pair per object.
Testing:
Ran core tests, which should exercise this heavily during query
execution.
Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
---
M be/src/common/object-pool.h
1 file changed, 15 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/5666/3
--
To view, visit http://gerrit.cloudera.org:8080/5666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-4752: make ObjectPool more efficient
......................................................................
IMPALA-4752: make ObjectPool more efficient
Previously it was implemented as a vector of pointers to
dynamically allocated wrapper objects, where each wrapper object
stored a pointer to the object and a pointer to a vtable, which
had a pointer to a destructor, which then calls the actual
object's destructor.
Instead of doing this, this patch changes ObjectPool to stores
the object pointer and function pointer inline in the vector.
This avoids an unnecessary malloc() and free() pair per object.
Testing:
Ran core tests, which should exercise this heavily during query
execution.
Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
Reviewed-on: http://gerrit.cloudera.org:8080/5666
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/common/object-pool.h
1 file changed, 15 insertions(+), 25 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Tim Armstrong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/5666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Thomas Tauber-Marshall, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5666
to look at the new patch set (#4).
Change subject: IMPALA-4752: make ObjectPool more efficient
......................................................................
IMPALA-4752: make ObjectPool more efficient
Previously it was implemented as a vector of pointers to
dynamically allocated wrapper objects, where each wrapper object
stored a pointer to the object and a pointer to a vtable, which
had a pointer to a destructor, which then calls the actual
object's destructor.
Instead of doing this, this patch changes ObjectPool to stores
the object pointer and function pointer inline in the vector.
This avoids an unnecessary malloc() and free() pair per object.
Testing:
Ran core tests, which should exercise this heavily during query
execution.
Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
---
M be/src/common/object-pool.h
1 file changed, 15 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/5666/4
--
To view, visit http://gerrit.cloudera.org:8080/5666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4752: make ObjectPool more efficient
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/5666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4752: make ObjectPool more efficient
......................................................................
Patch Set 4: Code-Review+2
Carry +2
--
To view, visit http://gerrit.cloudera.org:8080/5666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4752: make ObjectPool more efficient
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has uploaded a new patch set (#2).
Change subject: IMPALA-4752: make ObjectPool more efficient
......................................................................
IMPALA-4752: make ObjectPool more efficient
Previously it was implemented as a vector of pointers to
dynamically allocated wrapper objects, where each wrapper object
stored a pointer to the object and a pointer to a vtable, which
had a pointer to a destructor, which then calls the actual
object's destructor.
Instead of doing this, this patch changes ObjectPool to stores
the object pointer and function pointer inline in the vector.
This avoids an unnecessary malloc() and free() pair per object.
Testing:
Ran core tests, which should exercise this heavily during query
execution.
Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
---
M be/src/common/object-pool.h
1 file changed, 15 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/5666/2
--
To view, visit http://gerrit.cloudera.org:8080/5666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4752: make ObjectPool more efficient
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5666/3/be/src/common/object-pool.h
File be/src/common/object-pool.h:
PS3, Line 60: //
> '///' ?
Done
--
To view, visit http://gerrit.cloudera.org:8080/5666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4752: make ObjectPool more efficient
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5666/2/be/src/common/object-pool.h
File be/src/common/object-pool.h:
Line 43: Element elem{t, [](void* obj){ delete reinterpret_cast<T*>(obj); }};
> That link makes it sound like the braced initialization will still result i
I don't think it matters from a performance perspective - compilers are good at optimising stack-allocated structs. I just think it ended up being more concise this way.
--
To view, visit http://gerrit.cloudera.org:8080/5666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-4752: make ObjectPool more efficient
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5666/2/be/src/common/object-pool.h
File be/src/common/object-pool.h:
Line 43: objects_.emplace_back(
> It should be able to optimise this fine, since it's a plain struct and it j
That link makes it sound like the braced initialization will still result in a temporary plus a copy. To get in-place construction it seems to be necessary to define a c'tor. Alternatively you could use std::tuple, which has a c'tor defined.
--
To view, visit http://gerrit.cloudera.org:8080/5666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes