Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2740373002:
Cleaning up base::Value assignment. (Closed)
Description1 - Removing checks for self-assignment from base::Value.
Discussion on self-assignment:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/ZzfGfgruHxc
2 - Always destroying and creating instead of InternalMoveAssign.
BUG=646113
Review-Url: https://codereview.chromium.org/2740373002
Cr-Commit-Position: refs/heads/master@{#458101}
Committed: https://chromium.googlesource.com/chromium/src/+/534a0fda6258fcff015ea6a95f9fba981596c07e
Patch Set 1 : Removing check for self-assignment. #
Total comments: 2
Patch Set 2 : Always destroy in move. #
Total comments: 8
Patch Set 3 : Review, round 1. #Patch Set 4 : Review, round 2. #Patch Set 5 : Rebase. #Messages
Total messages: 45 (16 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Removing checks for self-assignment from base::Value. Discussion on self-assignment: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/ZzfGfgruHxc ========== to ========== Removing checks for self-assignment from base::Value. Discussion on self-assignment: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/ZzfGfgruHxc ==========
dyaroshev@yandex-team.ru changed reviewers: + brettw@chromium.org, dcheng@chromium.org, jdoerrie@chromium.org
On 2017/03/10 18:33:09, dyaroshev wrote: > mailto:dyaroshev@yandex-team.ru changed reviewers: > + mailto:brettw@chromium.org, mailto:dcheng@chromium.org, mailto:jdoerrie@chromium.org Since the self assignment case is very rare and self move assignment doesn't seem to be a valid thing - I think this is a better way to implement these operators. You might disagree. I suggest that to discuss this issue here https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/ZzfGfgruHxc and then be consistent with the decision.
https://codereview.chromium.org/2740373002/diff/20001/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2740373002/diff/20001/base/values.cc#newcode190 base/values.cc:190: InternalMoveAssignFromSameType(std::move(that)); We should just remove the branch entirely if we're DCHECKing.
https://codereview.chromium.org/2740373002/diff/20001/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2740373002/diff/20001/base/values.cc#newcode190 base/values.cc:190: InternalMoveAssignFromSameType(std::move(that)); On 2017/03/10 18:40:15, dcheng wrote: > We should just remove the branch entirely if we're DCHECKing. Done. std::variant (http://en.cppreference.com/w/cpp/utility/variant/operator%3D) (on which this is based) would check for matching types - but I've double checked - there does not seem to be a gain.
LGTM with one nit https://codereview.chromium.org/2740373002/diff/40001/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2740373002/diff/40001/base/values.cc#newcode188 base/values.cc:188: DCHECK(this != &that) << " attempt to self move assign."; Super minor nit: no space at the beginning of the string https://codereview.chromium.org/2740373002/diff/40001/base/values.cc#newcode530 base/values.cc:530: CHECK_EQ(type_, that.type_); While you're fixing this, do mind changing these to DCHECKs? I don't see a reason why they should be CHECKs.
On 2017/03/11 06:30:07, dcheng wrote: > LGTM with one nit > > https://codereview.chromium.org/2740373002/diff/40001/base/values.cc > File base/values.cc (right): > > https://codereview.chromium.org/2740373002/diff/40001/base/values.cc#newcode188 > base/values.cc:188: DCHECK(this != &that) << " attempt to self move assign."; > Super minor nit: no space at the beginning of the string > > https://codereview.chromium.org/2740373002/diff/40001/base/values.cc#newcode530 > base/values.cc:530: CHECK_EQ(type_, that.type_); > While you're fixing this, do mind changing these to DCHECKs? I don't see a > reason why they should be CHECKs. I don't agree with removing |InternalMoveAssignFromSameType|, as this results in a performance hit when we would be able to move assign from the same type. I wrote a simple perf test to test this behavior (http://crrev.com/2745733005) and found out that currently this is most pronounced for StringValues. Before this CL: *RESULT StringMoveAssign: Max Length: 2, TestOrder: ascending= 0 us *RESULT StringMoveAssign: Max Length: 4, TestOrder: ascending= 1 us *RESULT StringMoveAssign: Max Length: 8, TestOrder: ascending= 1 us *RESULT StringMoveAssign: Max Length: 16, TestOrder: ascending= 0 us *RESULT StringMoveAssign: Max Length: 32, TestOrder: ascending= 1 us *RESULT StringMoveAssign: Max Length: 64, TestOrder: ascending= 2 us *RESULT StringMoveAssign: Max Length: 128, TestOrder: ascending= 1 us *RESULT StringMoveAssign: Max Length: 256, TestOrder: ascending= 8 us *RESULT StringMoveAssign: Max Length: 512, TestOrder: ascending= 18 us *RESULT StringMoveAssign: Max Length: 1024, TestOrder: ascending= 41 us *RESULT StringMoveAssign: Max Length: 2048, TestOrder: ascending= 68 us *RESULT StringMoveAssign: Max Length: 4096, TestOrder: ascending= 141 us *RESULT StringMoveAssign: Max Length: 8192, TestOrder: ascending= 271 us *RESULT StringMoveAssign: Max Length: 16384, TestOrder: ascending= 579 us *RESULT StringMoveAssign: Max Length: 32768, TestOrder: ascending= 1142 us *RESULT StringMoveAssign: Max Length: 65536, TestOrder: ascending= 3702 us *RESULT StringMoveAssign: Max Length: 131072, TestOrder: ascending= 8990 us *RESULT StringMoveAssign: Max Length: 262144, TestOrder: ascending= 22800 us After this CL: *RESULT StringMoveAssign: Max Length: 1, TestOrder: ascending= 1 us *RESULT StringMoveAssign: Max Length: 2, TestOrder: ascending= 0 us *RESULT StringMoveAssign: Max Length: 4, TestOrder: ascending= 0 us *RESULT StringMoveAssign: Max Length: 8, TestOrder: ascending= 1 us *RESULT StringMoveAssign: Max Length: 16, TestOrder: ascending= 1 us *RESULT StringMoveAssign: Max Length: 32, TestOrder: ascending= 2 us *RESULT StringMoveAssign: Max Length: 64, TestOrder: ascending= 2 us *RESULT StringMoveAssign: Max Length: 128, TestOrder: ascending= 7 us *RESULT StringMoveAssign: Max Length: 256, TestOrder: ascending= 13 us *RESULT StringMoveAssign: Max Length: 512, TestOrder: ascending= 27 us *RESULT StringMoveAssign: Max Length: 1024, TestOrder: ascending= 54 us *RESULT StringMoveAssign: Max Length: 2048, TestOrder: ascending= 101 us *RESULT StringMoveAssign: Max Length: 4096, TestOrder: ascending= 176 us *RESULT StringMoveAssign: Max Length: 8192, TestOrder: ascending= 448 us *RESULT StringMoveAssign: Max Length: 16384, TestOrder: ascending= 883 us *RESULT StringMoveAssign: Max Length: 32768, TestOrder: ascending= 1965 us *RESULT StringMoveAssign: Max Length: 65536, TestOrder: ascending= 95284 us *RESULT StringMoveAssign: Max Length: 131072, TestOrder: ascending= 381933 us *RESULT StringMoveAssign: Max Length: 262144, TestOrder: ascending= 1359003 us Move Assigning 2^18 strings takes more than a second longer. Please feel free to do your own measurements using my code. Regarding the CHECKS: vabr@ introduced them recently to http://crrev.com/2728773005 in response to http://crbug.com/697817. In the CL description it says the following: """ The CL also introduces two more CHECKS in Internal*AssignFrom to prevent executing a desctructor on an arbitrary address. All the above are CHECKS, as opposed to DCHECKS, because if the checked conditions do not hold, execution of arbitrary parts of memory might be possible. """ This is the same reason as why we have CHECKs in the |Get*| accessors.
On 2017/03/11 13:09:49, jdoerrie wrote: > On 2017/03/11 06:30:07, dcheng wrote: > > LGTM with one nit > > > > https://codereview.chromium.org/2740373002/diff/40001/base/values.cc > > File base/values.cc (right): > > > > > https://codereview.chromium.org/2740373002/diff/40001/base/values.cc#newcode188 > > base/values.cc:188: DCHECK(this != &that) << " attempt to self move assign."; > > Super minor nit: no space at the beginning of the string > > > > > https://codereview.chromium.org/2740373002/diff/40001/base/values.cc#newcode530 > > base/values.cc:530: CHECK_EQ(type_, that.type_); > > While you're fixing this, do mind changing these to DCHECKs? I don't see a > > reason why they should be CHECKs. > > I don't agree with removing |InternalMoveAssignFromSameType|, as this results in > a performance hit when we would be able to move assign from the same type. > I wrote a simple perf test to test this behavior (http://crrev.com/2745733005) > and found out that currently this is most pronounced for StringValues. > > Before this CL: > *RESULT StringMoveAssign: Max Length: 2, TestOrder: ascending= 0 us > *RESULT StringMoveAssign: Max Length: 4, TestOrder: ascending= 1 us > *RESULT StringMoveAssign: Max Length: 8, TestOrder: ascending= 1 us > *RESULT StringMoveAssign: Max Length: 16, TestOrder: ascending= 0 us > *RESULT StringMoveAssign: Max Length: 32, TestOrder: ascending= 1 us > *RESULT StringMoveAssign: Max Length: 64, TestOrder: ascending= 2 us > *RESULT StringMoveAssign: Max Length: 128, TestOrder: ascending= 1 us > *RESULT StringMoveAssign: Max Length: 256, TestOrder: ascending= 8 us > *RESULT StringMoveAssign: Max Length: 512, TestOrder: ascending= 18 us > *RESULT StringMoveAssign: Max Length: 1024, TestOrder: ascending= 41 us > *RESULT StringMoveAssign: Max Length: 2048, TestOrder: ascending= 68 us > *RESULT StringMoveAssign: Max Length: 4096, TestOrder: ascending= 141 us > *RESULT StringMoveAssign: Max Length: 8192, TestOrder: ascending= 271 us > *RESULT StringMoveAssign: Max Length: 16384, TestOrder: ascending= 579 us > *RESULT StringMoveAssign: Max Length: 32768, TestOrder: ascending= 1142 us > *RESULT StringMoveAssign: Max Length: 65536, TestOrder: ascending= 3702 us > *RESULT StringMoveAssign: Max Length: 131072, TestOrder: ascending= 8990 us > *RESULT StringMoveAssign: Max Length: 262144, TestOrder: ascending= 22800 us > > After this CL: > *RESULT StringMoveAssign: Max Length: 1, TestOrder: ascending= 1 us > *RESULT StringMoveAssign: Max Length: 2, TestOrder: ascending= 0 us > *RESULT StringMoveAssign: Max Length: 4, TestOrder: ascending= 0 us > *RESULT StringMoveAssign: Max Length: 8, TestOrder: ascending= 1 us > *RESULT StringMoveAssign: Max Length: 16, TestOrder: ascending= 1 us > *RESULT StringMoveAssign: Max Length: 32, TestOrder: ascending= 2 us > *RESULT StringMoveAssign: Max Length: 64, TestOrder: ascending= 2 us > *RESULT StringMoveAssign: Max Length: 128, TestOrder: ascending= 7 us > *RESULT StringMoveAssign: Max Length: 256, TestOrder: ascending= 13 us > *RESULT StringMoveAssign: Max Length: 512, TestOrder: ascending= 27 us > *RESULT StringMoveAssign: Max Length: 1024, TestOrder: ascending= 54 us > *RESULT StringMoveAssign: Max Length: 2048, TestOrder: ascending= 101 us > *RESULT StringMoveAssign: Max Length: 4096, TestOrder: ascending= 176 us > *RESULT StringMoveAssign: Max Length: 8192, TestOrder: ascending= 448 us > *RESULT StringMoveAssign: Max Length: 16384, TestOrder: ascending= 883 us > *RESULT StringMoveAssign: Max Length: 32768, TestOrder: ascending= 1965 us > *RESULT StringMoveAssign: Max Length: 65536, TestOrder: ascending= 95284 us > *RESULT StringMoveAssign: Max Length: 131072, TestOrder: ascending= 381933 us > *RESULT StringMoveAssign: Max Length: 262144, TestOrder: ascending= 1359003 us > > Move Assigning 2^18 strings takes more than a second longer. Please feel free to > do your own measurements using my code. > > Regarding the CHECKS: vabr@ introduced them recently to > http://crrev.com/2728773005 in response to http://crbug.com/697817. > > In the CL description it says the following: > """ > The CL also introduces two more CHECKS in Internal*AssignFrom to prevent > executing a desctructor on an arbitrary address. > > All the above are CHECKS, as opposed to DCHECKS, because if the checked > conditions do not hold, execution of arbitrary parts of memory might be > possible. > """ > > This is the same reason as why we have CHECKs in the |Get*| accessors. These are very interesting results. What platform/compiler you were measuring on?
On 2017/03/11 23:52:10, dyaroshev wrote: > On 2017/03/11 13:09:49, jdoerrie wrote: > > On 2017/03/11 06:30:07, dcheng wrote: > > > LGTM with one nit > > > > > > https://codereview.chromium.org/2740373002/diff/40001/base/values.cc > > > File base/values.cc (right): > > > > > > > > > https://codereview.chromium.org/2740373002/diff/40001/base/values.cc#newcode188 > > > base/values.cc:188: DCHECK(this != &that) << " attempt to self move > assign."; > > > Super minor nit: no space at the beginning of the string > > > > > > > > > https://codereview.chromium.org/2740373002/diff/40001/base/values.cc#newcode530 > > > base/values.cc:530: CHECK_EQ(type_, that.type_); > > > While you're fixing this, do mind changing these to DCHECKs? I don't see a > > > reason why they should be CHECKs. > > > > I don't agree with removing |InternalMoveAssignFromSameType|, as this results > in > > a performance hit when we would be able to move assign from the same type. > > I wrote a simple perf test to test this behavior (http://crrev.com/2745733005) > > and found out that currently this is most pronounced for StringValues. > > > > Before this CL: > > *RESULT StringMoveAssign: Max Length: 2, TestOrder: ascending= 0 us > > *RESULT StringMoveAssign: Max Length: 4, TestOrder: ascending= 1 us > > *RESULT StringMoveAssign: Max Length: 8, TestOrder: ascending= 1 us > > *RESULT StringMoveAssign: Max Length: 16, TestOrder: ascending= 0 us > > *RESULT StringMoveAssign: Max Length: 32, TestOrder: ascending= 1 us > > *RESULT StringMoveAssign: Max Length: 64, TestOrder: ascending= 2 us > > *RESULT StringMoveAssign: Max Length: 128, TestOrder: ascending= 1 us > > *RESULT StringMoveAssign: Max Length: 256, TestOrder: ascending= 8 us > > *RESULT StringMoveAssign: Max Length: 512, TestOrder: ascending= 18 us > > *RESULT StringMoveAssign: Max Length: 1024, TestOrder: ascending= 41 us > > *RESULT StringMoveAssign: Max Length: 2048, TestOrder: ascending= 68 us > > *RESULT StringMoveAssign: Max Length: 4096, TestOrder: ascending= 141 us > > *RESULT StringMoveAssign: Max Length: 8192, TestOrder: ascending= 271 us > > *RESULT StringMoveAssign: Max Length: 16384, TestOrder: ascending= 579 us > > *RESULT StringMoveAssign: Max Length: 32768, TestOrder: ascending= 1142 us > > *RESULT StringMoveAssign: Max Length: 65536, TestOrder: ascending= 3702 us > > *RESULT StringMoveAssign: Max Length: 131072, TestOrder: ascending= 8990 us > > *RESULT StringMoveAssign: Max Length: 262144, TestOrder: ascending= 22800 us > > > > After this CL: > > *RESULT StringMoveAssign: Max Length: 1, TestOrder: ascending= 1 us > > *RESULT StringMoveAssign: Max Length: 2, TestOrder: ascending= 0 us > > *RESULT StringMoveAssign: Max Length: 4, TestOrder: ascending= 0 us > > *RESULT StringMoveAssign: Max Length: 8, TestOrder: ascending= 1 us > > *RESULT StringMoveAssign: Max Length: 16, TestOrder: ascending= 1 us > > *RESULT StringMoveAssign: Max Length: 32, TestOrder: ascending= 2 us > > *RESULT StringMoveAssign: Max Length: 64, TestOrder: ascending= 2 us > > *RESULT StringMoveAssign: Max Length: 128, TestOrder: ascending= 7 us > > *RESULT StringMoveAssign: Max Length: 256, TestOrder: ascending= 13 us > > *RESULT StringMoveAssign: Max Length: 512, TestOrder: ascending= 27 us > > *RESULT StringMoveAssign: Max Length: 1024, TestOrder: ascending= 54 us > > *RESULT StringMoveAssign: Max Length: 2048, TestOrder: ascending= 101 us > > *RESULT StringMoveAssign: Max Length: 4096, TestOrder: ascending= 176 us > > *RESULT StringMoveAssign: Max Length: 8192, TestOrder: ascending= 448 us > > *RESULT StringMoveAssign: Max Length: 16384, TestOrder: ascending= 883 us > > *RESULT StringMoveAssign: Max Length: 32768, TestOrder: ascending= 1965 us > > *RESULT StringMoveAssign: Max Length: 65536, TestOrder: ascending= 95284 us > > *RESULT StringMoveAssign: Max Length: 131072, TestOrder: ascending= 381933 us > > *RESULT StringMoveAssign: Max Length: 262144, TestOrder: ascending= 1359003 us > > > > Move Assigning 2^18 strings takes more than a second longer. Please feel free > to > > do your own measurements using my code. > > > > Regarding the CHECKS: vabr@ introduced them recently to > > http://crrev.com/2728773005 in response to http://crbug.com/697817. > > > > In the CL description it says the following: > > """ > > The CL also introduces two more CHECKS in Internal*AssignFrom to prevent > > executing a desctructor on an arbitrary address. > > > > All the above are CHECKS, as opposed to DCHECKS, because if the checked > > conditions do not hold, execution of arbitrary parts of memory might be > > possible. > > """ > > > > This is the same reason as why we have CHECKs in the |Get*| accessors. > > These are very interesting results. What platform/compiler you were measuring > on? I think I'm OK with keeping InternalMoveAssignFromSameType, but the checks really shouldn't be CHECKs. We already check the condition before we call.
jdoerrie@chromium.org changed reviewers: + vabr@chromium.org
> These are very interesting results. What platform/compiler you were measuring
> on?
I obtained the results on a 64 bit Ubuntu 14.04.5 LTS using clang as a compiler.
> I think I'm OK with keeping InternalMoveAssignFromSameType, but the checks
> really shouldn't be CHECKs. We already check the condition before we call.
Given that the |Internal{Copy,Move}AssignFromSameType| APIs are private and we
have control of all callsites I would be okay to change them into DCHECKs so
that we don't have any runtime overhead in Release builds. However, I'm still
curious to hear what vabr@ has to say regarding this.
I fail to see the issue with the CHECKs -- they are simple int comparisons, are we sure they are in a hotspot? I do see an issue with removing them, because while the code today looks like callsites check the types, if somebody makes a mistake, interesting new attacks are possible. Having said that, I'm neither a //base OWNER, not a security engineer, so feel free to override me. Cheers, Vaclav https://codereview.chromium.org/2740373002/diff/40001/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2740373002/diff/40001/base/values.cc#newcode179 base/values.cc:179: // this is not a self assignment because the type_ doesn't match. this -> This https://codereview.chromium.org/2740373002/diff/40001/base/values.cc#newcode530 base/values.cc:530: CHECK_EQ(type_, that.type_); On 2017/03/11 06:30:07, dcheng wrote: > While you're fixing this, do mind changing these to DCHECKs? I don't see a > reason why they should be CHECKs. The reason is potential execution of random pieces of memory. Please do not remove the checks. Please see https://codereview.chromium.org/2728773005/ for more context.
On 2017/03/11 13:09:49, jdoerrie wrote: > On 2017/03/11 06:30:07, dcheng wrote: > > LGTM with one nit > > > > https://codereview.chromium.org/2740373002/diff/40001/base/values.cc > > File base/values.cc (right): > > > > > https://codereview.chromium.org/2740373002/diff/40001/base/values.cc#newcode188 > > base/values.cc:188: DCHECK(this != &that) << " attempt to self move assign."; > > Super minor nit: no space at the beginning of the string > > > > > https://codereview.chromium.org/2740373002/diff/40001/base/values.cc#newcode530 > > base/values.cc:530: CHECK_EQ(type_, that.type_); > > While you're fixing this, do mind changing these to DCHECKs? I don't see a > > reason why they should be CHECKs. > > I don't agree with removing |InternalMoveAssignFromSameType|, as this results in > a performance hit when we would be able to move assign from the same type. > I wrote a simple perf test to test this behavior (http://crrev.com/2745733005) > and found out that currently this is most pronounced for StringValues. > > Before this CL: > *RESULT StringMoveAssign: Max Length: 2, TestOrder: ascending= 0 us > *RESULT StringMoveAssign: Max Length: 4, TestOrder: ascending= 1 us > *RESULT StringMoveAssign: Max Length: 8, TestOrder: ascending= 1 us > *RESULT StringMoveAssign: Max Length: 16, TestOrder: ascending= 0 us > *RESULT StringMoveAssign: Max Length: 32, TestOrder: ascending= 1 us > *RESULT StringMoveAssign: Max Length: 64, TestOrder: ascending= 2 us > *RESULT StringMoveAssign: Max Length: 128, TestOrder: ascending= 1 us > *RESULT StringMoveAssign: Max Length: 256, TestOrder: ascending= 8 us > *RESULT StringMoveAssign: Max Length: 512, TestOrder: ascending= 18 us > *RESULT StringMoveAssign: Max Length: 1024, TestOrder: ascending= 41 us > *RESULT StringMoveAssign: Max Length: 2048, TestOrder: ascending= 68 us > *RESULT StringMoveAssign: Max Length: 4096, TestOrder: ascending= 141 us > *RESULT StringMoveAssign: Max Length: 8192, TestOrder: ascending= 271 us > *RESULT StringMoveAssign: Max Length: 16384, TestOrder: ascending= 579 us > *RESULT StringMoveAssign: Max Length: 32768, TestOrder: ascending= 1142 us > *RESULT StringMoveAssign: Max Length: 65536, TestOrder: ascending= 3702 us > *RESULT StringMoveAssign: Max Length: 131072, TestOrder: ascending= 8990 us > *RESULT StringMoveAssign: Max Length: 262144, TestOrder: ascending= 22800 us > > After this CL: > *RESULT StringMoveAssign: Max Length: 1, TestOrder: ascending= 1 us > *RESULT StringMoveAssign: Max Length: 2, TestOrder: ascending= 0 us > *RESULT StringMoveAssign: Max Length: 4, TestOrder: ascending= 0 us > *RESULT StringMoveAssign: Max Length: 8, TestOrder: ascending= 1 us > *RESULT StringMoveAssign: Max Length: 16, TestOrder: ascending= 1 us > *RESULT StringMoveAssign: Max Length: 32, TestOrder: ascending= 2 us > *RESULT StringMoveAssign: Max Length: 64, TestOrder: ascending= 2 us > *RESULT StringMoveAssign: Max Length: 128, TestOrder: ascending= 7 us > *RESULT StringMoveAssign: Max Length: 256, TestOrder: ascending= 13 us > *RESULT StringMoveAssign: Max Length: 512, TestOrder: ascending= 27 us > *RESULT StringMoveAssign: Max Length: 1024, TestOrder: ascending= 54 us > *RESULT StringMoveAssign: Max Length: 2048, TestOrder: ascending= 101 us > *RESULT StringMoveAssign: Max Length: 4096, TestOrder: ascending= 176 us > *RESULT StringMoveAssign: Max Length: 8192, TestOrder: ascending= 448 us > *RESULT StringMoveAssign: Max Length: 16384, TestOrder: ascending= 883 us > *RESULT StringMoveAssign: Max Length: 32768, TestOrder: ascending= 1965 us > *RESULT StringMoveAssign: Max Length: 65536, TestOrder: ascending= 95284 us > *RESULT StringMoveAssign: Max Length: 131072, TestOrder: ascending= 381933 us > *RESULT StringMoveAssign: Max Length: 262144, TestOrder: ascending= 1359003 us > > Move Assigning 2^18 strings takes more than a second longer. Please feel free to > do your own measurements using my code. > > Regarding the CHECKS: vabr@ introduced them recently to > http://crrev.com/2728773005 in response to http://crbug.com/697817. > > In the CL description it says the following: > """ > The CL also introduces two more CHECKS in Internal*AssignFrom to prevent > executing a desctructor on an arbitrary address. > > All the above are CHECKS, as opposed to DCHECKS, because if the checked > conditions do not hold, execution of arbitrary parts of memory might be > possible. > """ > > This is the same reason as why we have CHECKs in the |Get*| accessors. Ok, these are my results - they tell the opposite. Mac, clang. Before CL: [ RUN ] ValuePerfTest.StringAssignTest *RESULT StringMoveAssign: Max Length: 1, TestOrder: ascending= 11 us *RESULT StringMoveAssign: Max Length: 2, TestOrder: ascending= 1 us *RESULT StringMoveAssign: Max Length: 4, TestOrder: ascending= 0 us *RESULT StringMoveAssign: Max Length: 8, TestOrder: ascending= 0 us *RESULT StringMoveAssign: Max Length: 16, TestOrder: ascending= 2 us *RESULT StringMoveAssign: Max Length: 32, TestOrder: ascending= 0 us *RESULT StringMoveAssign: Max Length: 64, TestOrder: ascending= 4 us *RESULT StringMoveAssign: Max Length: 128, TestOrder: ascending= 12 us *RESULT StringMoveAssign: Max Length: 256, TestOrder: ascending= 26 us *RESULT StringMoveAssign: Max Length: 512, TestOrder: ascending= 56 us *RESULT StringMoveAssign: Max Length: 1024, TestOrder: ascending= 120 us *RESULT StringMoveAssign: Max Length: 2048, TestOrder: ascending= 201 us *RESULT StringMoveAssign: Max Length: 4096, TestOrder: ascending= 391 us *RESULT StringMoveAssign: Max Length: 8192, TestOrder: ascending= 733 us *RESULT StringMoveAssign: Max Length: 16384, TestOrder: ascending= 1407 us *RESULT StringMoveAssign: Max Length: 32768, TestOrder: ascending= 2834 us *RESULT StringMoveAssign: Max Length: 65536, TestOrder: ascending= 7300 us *RESULT StringMoveAssign: Max Length: 131072, TestOrder: ascending= 13653 us *RESULT StringMoveAssign: Max Length: 262144, TestOrder: ascending= 75326 us After CL: *RESULT StringMoveAssign: Max Length: 1, TestOrder: ascending= 12 us *RESULT StringMoveAssign: Max Length: 2, TestOrder: ascending= 0 us *RESULT StringMoveAssign: Max Length: 4, TestOrder: ascending= 0 us *RESULT StringMoveAssign: Max Length: 8, TestOrder: ascending= 0 us *RESULT StringMoveAssign: Max Length: 16, TestOrder: ascending= 1 us *RESULT StringMoveAssign: Max Length: 32, TestOrder: ascending= 1 us *RESULT StringMoveAssign: Max Length: 64, TestOrder: ascending= 3 us *RESULT StringMoveAssign: Max Length: 128, TestOrder: ascending= 11 us *RESULT StringMoveAssign: Max Length: 256, TestOrder: ascending= 31 us *RESULT StringMoveAssign: Max Length: 512, TestOrder: ascending= 59 us *RESULT StringMoveAssign: Max Length: 1024, TestOrder: ascending= 135 us *RESULT StringMoveAssign: Max Length: 2048, TestOrder: ascending= 201 us *RESULT StringMoveAssign: Max Length: 4096, TestOrder: ascending= 342 us *RESULT StringMoveAssign: Max Length: 8192, TestOrder: ascending= 629 us *RESULT StringMoveAssign: Max Length: 16384, TestOrder: ascending= 1216 us *RESULT StringMoveAssign: Max Length: 32768, TestOrder: ascending= 2463 us *RESULT StringMoveAssign: Max Length: 65536, TestOrder: ascending= 5870 us *RESULT StringMoveAssign: Max Length: 131072, TestOrder: ascending= 11550 us *RESULT StringMoveAssign: Max Length: 262144, TestOrder: ascending= 67454 us ////////////////////////////////////////
https://codereview.chromium.org/2740373002/diff/40001/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2740373002/diff/40001/base/values.cc#newcode179 base/values.cc:179: // this is not a self assignment because the type_ doesn't match. On 2017/03/13 10:56:01, vabr (Chromium) wrote: > this -> This Done. https://codereview.chromium.org/2740373002/diff/40001/base/values.cc#newcode188 base/values.cc:188: DCHECK(this != &that) << " attempt to self move assign."; On 2017/03/11 06:30:07, dcheng wrote: > Super minor nit: no space at the beginning of the string Done. https://codereview.chromium.org/2740373002/diff/40001/base/values.cc#newcode530 base/values.cc:530: CHECK_EQ(type_, that.type_); On 2017/03/13 10:56:01, vabr (Chromium) wrote: > On 2017/03/11 06:30:07, dcheng wrote: > > While you're fixing this, do mind changing these to DCHECKs? I don't see a > > reason why they should be CHECKs. > > The reason is potential execution of random pieces of memory. Please do not > remove the checks. Please see https://codereview.chromium.org/2728773005/ for > more context. I see - you worried about slicing causing execution of random code. Would you think that it's ok to remove these CHECKs after value conversion is done? It has a measurable performance impact (not very big though).
https://codereview.chromium.org/2740373002/diff/40001/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2740373002/diff/40001/base/values.cc#newcode530 base/values.cc:530: CHECK_EQ(type_, that.type_); On 2017/03/13 21:15:42, dyaroshev wrote: > On 2017/03/13 10:56:01, vabr (Chromium) wrote: > > On 2017/03/11 06:30:07, dcheng wrote: > > > While you're fixing this, do mind changing these to DCHECKs? I don't see a > > > reason why they should be CHECKs. > > > > The reason is potential execution of random pieces of memory. Please do not > > remove the checks. Please see https://codereview.chromium.org/2728773005/ for > > more context. > > I see - you worried about slicing causing execution of random code. > Would you think that it's ok to remove these CHECKs after value conversion is > done? > It has a measurable performance impact (not very big though). While it's true that we should be using CHECK when there are security implications... this is an internal function that's called in one place, by one thing. An internal class helper should be allowed to assume that its own implementation will satisfy the preconditions, and use DCHECK to enforce it.
On 2017/03/14 05:08:02, dcheng wrote: > https://codereview.chromium.org/2740373002/diff/40001/base/values.cc > File base/values.cc (right): > > https://codereview.chromium.org/2740373002/diff/40001/base/values.cc#newcode530 > base/values.cc:530: CHECK_EQ(type_, that.type_); > On 2017/03/13 21:15:42, dyaroshev wrote: > > On 2017/03/13 10:56:01, vabr (Chromium) wrote: > > > On 2017/03/11 06:30:07, dcheng wrote: > > > > While you're fixing this, do mind changing these to DCHECKs? I don't see a > > > > reason why they should be CHECKs. > > > > > > The reason is potential execution of random pieces of memory. Please do not > > > remove the checks. Please see https://codereview.chromium.org/2728773005/ > for > > > more context. > > > > I see - you worried about slicing causing execution of random code. > > Would you think that it's ok to remove these CHECKs after value conversion is > > done? > > It has a measurable performance impact (not very big though). I don't think this is due to slicing, it is more the issue of having the union of storage inside base::Value. That's not going away even if base::Value will no longer be inherited from. Slicing is an issue for cases like DictionaryValue::Swap. There I also put some CHECKS, and those are fine to disappear once DictionaryValue gets collapsed into Value. > > While it's true that we should be using CHECK when there are security > implications... this is an internal function that's called in one place, by one > thing. An internal class helper should be allowed to assume that its own > implementation will satisfy the preconditions, and use DCHECK to enforce it. I yield to your argumentation, dcheng@. As a security team member, you have a better understanding of what is the bar for CHECKs in Chromium code. Cheers, Vaclav
> Ok, these are my results - they tell the opposite. > Mac, clang. > > Before CL: > > [ RUN ] ValuePerfTest.StringAssignTest > *RESULT StringMoveAssign: Max Length: 1, TestOrder: ascending= 11 us > *RESULT StringMoveAssign: Max Length: 2, TestOrder: ascending= 1 us > *RESULT StringMoveAssign: Max Length: 4, TestOrder: ascending= 0 us > *RESULT StringMoveAssign: Max Length: 8, TestOrder: ascending= 0 us > *RESULT StringMoveAssign: Max Length: 16, TestOrder: ascending= 2 us > *RESULT StringMoveAssign: Max Length: 32, TestOrder: ascending= 0 us > *RESULT StringMoveAssign: Max Length: 64, TestOrder: ascending= 4 us > *RESULT StringMoveAssign: Max Length: 128, TestOrder: ascending= 12 us > *RESULT StringMoveAssign: Max Length: 256, TestOrder: ascending= 26 us > *RESULT StringMoveAssign: Max Length: 512, TestOrder: ascending= 56 us > *RESULT StringMoveAssign: Max Length: 1024, TestOrder: ascending= 120 us > *RESULT StringMoveAssign: Max Length: 2048, TestOrder: ascending= 201 us > *RESULT StringMoveAssign: Max Length: 4096, TestOrder: ascending= 391 us > *RESULT StringMoveAssign: Max Length: 8192, TestOrder: ascending= 733 us > *RESULT StringMoveAssign: Max Length: 16384, TestOrder: ascending= 1407 us > *RESULT StringMoveAssign: Max Length: 32768, TestOrder: ascending= 2834 us > *RESULT StringMoveAssign: Max Length: 65536, TestOrder: ascending= 7300 us > *RESULT StringMoveAssign: Max Length: 131072, TestOrder: ascending= 13653 us > *RESULT StringMoveAssign: Max Length: 262144, TestOrder: ascending= 75326 us > > After CL: > *RESULT StringMoveAssign: Max Length: 1, TestOrder: ascending= 12 us > *RESULT StringMoveAssign: Max Length: 2, TestOrder: ascending= 0 us > *RESULT StringMoveAssign: Max Length: 4, TestOrder: ascending= 0 us > *RESULT StringMoveAssign: Max Length: 8, TestOrder: ascending= 0 us > *RESULT StringMoveAssign: Max Length: 16, TestOrder: ascending= 1 us > *RESULT StringMoveAssign: Max Length: 32, TestOrder: ascending= 1 us > *RESULT StringMoveAssign: Max Length: 64, TestOrder: ascending= 3 us > *RESULT StringMoveAssign: Max Length: 128, TestOrder: ascending= 11 us > *RESULT StringMoveAssign: Max Length: 256, TestOrder: ascending= 31 us > *RESULT StringMoveAssign: Max Length: 512, TestOrder: ascending= 59 us > *RESULT StringMoveAssign: Max Length: 1024, TestOrder: ascending= 135 us > *RESULT StringMoveAssign: Max Length: 2048, TestOrder: ascending= 201 us > *RESULT StringMoveAssign: Max Length: 4096, TestOrder: ascending= 342 us > *RESULT StringMoveAssign: Max Length: 8192, TestOrder: ascending= 629 us > *RESULT StringMoveAssign: Max Length: 16384, TestOrder: ascending= 1216 us > *RESULT StringMoveAssign: Max Length: 32768, TestOrder: ascending= 2463 us > *RESULT StringMoveAssign: Max Length: 65536, TestOrder: ascending= 5870 us > *RESULT StringMoveAssign: Max Length: 131072, TestOrder: ascending= 11550 us > *RESULT StringMoveAssign: Max Length: 262144, TestOrder: ascending= 67454 us > > //////////////////////////////////////// I reran the benchmark on my machine using fixed sized strings, it looked like some of the string construction time leaked into the measurements before. This is what I get now: Using |this != &that| and |InternalMoveAssignFromSameType| (ie before this CL): *RESULT StringMoveAssign: Max Length: 1, TestOrder: ascending= 2 us *RESULT StringMoveAssign: Max Length: 2, TestOrder: ascending= 0 us *RESULT StringMoveAssign: Max Length: 4, TestOrder: ascending= 0 us *RESULT StringMoveAssign: Max Length: 8, TestOrder: ascending= 0 us *RESULT StringMoveAssign: Max Length: 16, TestOrder: ascending= 0 us *RESULT StringMoveAssign: Max Length: 32, TestOrder: ascending= 1 us *RESULT StringMoveAssign: Max Length: 64, TestOrder: ascending= 2 us *RESULT StringMoveAssign: Max Length: 128, TestOrder: ascending= 4 us *RESULT StringMoveAssign: Max Length: 256, TestOrder: ascending= 9 us *RESULT StringMoveAssign: Max Length: 512, TestOrder: ascending= 18 us *RESULT StringMoveAssign: Max Length: 1024, TestOrder: ascending= 38 us *RESULT StringMoveAssign: Max Length: 2048, TestOrder: ascending= 60 us *RESULT StringMoveAssign: Max Length: 4096, TestOrder: ascending= 135 us *RESULT StringMoveAssign: Max Length: 8192, TestOrder: ascending= 273 us *RESULT StringMoveAssign: Max Length: 16384, TestOrder: ascending= 553 us *RESULT StringMoveAssign: Max Length: 32768, TestOrder: ascending= 1106 us *RESULT StringMoveAssign: Max Length: 65536, TestOrder: ascending= 2134 us *RESULT StringMoveAssign: Max Length: 131072, TestOrder: ascending= 4356 us *RESULT StringMoveAssign: Max Length: 262144, TestOrder: ascending= 6649 us *RESULT StringMoveAssign: Max Length: 524288, TestOrder: ascending= 13031 us *RESULT StringMoveAssign: Max Length: 1048576, TestOrder: ascending= 26125 us Just using |InternalMoveAssignFromSameType| (ie just dropping |this != &that|): *RESULT StringMoveAssign: Max Length: 1, TestOrder: ascending= 1 us *RESULT StringMoveAssign: Max Length: 2, TestOrder: ascending= 0 us *RESULT StringMoveAssign: Max Length: 4, TestOrder: ascending= 0 us *RESULT StringMoveAssign: Max Length: 8, TestOrder: ascending= 0 us *RESULT StringMoveAssign: Max Length: 16, TestOrder: ascending= 1 us *RESULT StringMoveAssign: Max Length: 32, TestOrder: ascending= 1 us *RESULT StringMoveAssign: Max Length: 64, TestOrder: ascending= 2 us *RESULT StringMoveAssign: Max Length: 128, TestOrder: ascending= 4 us *RESULT StringMoveAssign: Max Length: 256, TestOrder: ascending= 7 us *RESULT StringMoveAssign: Max Length: 512, TestOrder: ascending= 18 us *RESULT StringMoveAssign: Max Length: 1024, TestOrder: ascending= 32 us *RESULT StringMoveAssign: Max Length: 2048, TestOrder: ascending= 72 us *RESULT StringMoveAssign: Max Length: 4096, TestOrder: ascending= 123 us *RESULT StringMoveAssign: Max Length: 8192, TestOrder: ascending= 266 us *RESULT StringMoveAssign: Max Length: 16384, TestOrder: ascending= 530 us *RESULT StringMoveAssign: Max Length: 32768, TestOrder: ascending= 1104 us *RESULT StringMoveAssign: Max Length: 65536, TestOrder: ascending= 2132 us *RESULT StringMoveAssign: Max Length: 131072, TestOrder: ascending= 4299 us *RESULT StringMoveAssign: Max Length: 262144, TestOrder: ascending= 7801 us *RESULT StringMoveAssign: Max Length: 524288, TestOrder: ascending= 13024 us *RESULT StringMoveAssign: Max Length: 1048576, TestOrder: ascending= 25862 us Using |Cleanup| and |MoveConstructFrom| (ie after the current state of this CL): *RESULT StringMoveAssign: Max Length: 1, TestOrder: ascending= 1 us *RESULT StringMoveAssign: Max Length: 2, TestOrder: ascending= 0 us *RESULT StringMoveAssign: Max Length: 4, TestOrder: ascending= 0 us *RESULT StringMoveAssign: Max Length: 8, TestOrder: ascending= 0 us *RESULT StringMoveAssign: Max Length: 16, TestOrder: ascending= 2 us *RESULT StringMoveAssign: Max Length: 32, TestOrder: ascending= 2 us *RESULT StringMoveAssign: Max Length: 64, TestOrder: ascending= 2 us *RESULT StringMoveAssign: Max Length: 128, TestOrder: ascending= 5 us *RESULT StringMoveAssign: Max Length: 256, TestOrder: ascending= 14 us *RESULT StringMoveAssign: Max Length: 512, TestOrder: ascending= 31 us *RESULT StringMoveAssign: Max Length: 1024, TestOrder: ascending= 75 us *RESULT StringMoveAssign: Max Length: 2048, TestOrder: ascending= 96 us *RESULT StringMoveAssign: Max Length: 4096, TestOrder: ascending= 251 us *RESULT StringMoveAssign: Max Length: 8192, TestOrder: ascending= 363 us *RESULT StringMoveAssign: Max Length: 16384, TestOrder: ascending= 625 us *RESULT StringMoveAssign: Max Length: 32768, TestOrder: ascending= 1378 us *RESULT StringMoveAssign: Max Length: 65536, TestOrder: ascending= 2692 us *RESULT StringMoveAssign: Max Length: 131072, TestOrder: ascending= 5523 us *RESULT StringMoveAssign: Max Length: 262144, TestOrder: ascending= 10462 us *RESULT StringMoveAssign: Max Length: 524288, TestOrder: ascending= 21138 us *RESULT StringMoveAssign: Max Length: 1048576, TestOrder: ascending= 41099 us This again is on a 64 bit Linux with Clang, I currently don't have easy access to other platforms, unfortunately. Judging by this results there is a performance hit of about ~60% when dropping |InternalMoveAssignFromSameType|. This makes sense to me, because we always do a memory allocation in that case (https://cs.chromium.org/chromium/src/base/memory/manual_constructor.h?l=57). It's surprising to me that your results show the exact opposite, though.
On 2017/03/14 08:41:04, jdoerrie wrote: > > This again is on a 64 bit Linux with Clang, I currently don't have easy access > to other platforms, unfortunately. > Judging by this results there is a performance hit of about ~60% when dropping > |InternalMoveAssignFromSameType|. > This makes sense to me, because we always do a memory allocation in that case > (https://cs.chromium.org/chromium/src/base/memory/manual_constructor.h?l=57). > I'm not sure what you mean - this is placement new, it just calls the constructor of std::string, move constructor of std::string should not call new. Oh! I - think I have an idea! You test on linux, right? We have some issues with the standard library on linux - it's older and does not support C++11 fully. Can you please check if the standard library you are using has move constructors in std::string?
On 2017/03/14 08:13:46, vabr (Chromium) wrote: > On 2017/03/14 05:08:02, dcheng wrote: > > https://codereview.chromium.org/2740373002/diff/40001/base/values.cc > > File base/values.cc (right): > > > > > https://codereview.chromium.org/2740373002/diff/40001/base/values.cc#newcode530 > > base/values.cc:530: CHECK_EQ(type_, that.type_); > > On 2017/03/13 21:15:42, dyaroshev wrote: > > > On 2017/03/13 10:56:01, vabr (Chromium) wrote: > > > > On 2017/03/11 06:30:07, dcheng wrote: > > > > > While you're fixing this, do mind changing these to DCHECKs? I don't see > a > > > > > reason why they should be CHECKs. > > > > > > > > The reason is potential execution of random pieces of memory. Please do > not > > > > remove the checks. Please see https://codereview.chromium.org/2728773005/ > > for > > > > more context. > > > > > > I see - you worried about slicing causing execution of random code. > > > Would you think that it's ok to remove these CHECKs after value conversion > is > > > done? > > > It has a measurable performance impact (not very big though). > > I don't think this is due to slicing, it is more the issue of having the union > of storage inside base::Value. That's not going away even if base::Value will no > longer be inherited from. Well, I'm looking at your example. If DictionaryValue is a part of Value, I think this code: 1: DictionaryValue d; 2: Value *p = &d; 3: *p = Value(1.234); 4: DictionaryValue().Swap(&d); It seems to be OK, since it's just: Value d; Value* p = &d; *p = Value(1.234); Value().Swap(&d); p is now an empty value, is it not?
On 2017/03/15 20:04:48, dyaroshev wrote: > On 2017/03/14 08:13:46, vabr (Chromium) wrote: > > On 2017/03/14 05:08:02, dcheng wrote: > > > https://codereview.chromium.org/2740373002/diff/40001/base/values.cc > > > File base/values.cc (right): > > > > > > > > > https://codereview.chromium.org/2740373002/diff/40001/base/values.cc#newcode530 > > > base/values.cc:530: CHECK_EQ(type_, that.type_); > > > On 2017/03/13 21:15:42, dyaroshev wrote: > > > > On 2017/03/13 10:56:01, vabr (Chromium) wrote: > > > > > On 2017/03/11 06:30:07, dcheng wrote: > > > > > > While you're fixing this, do mind changing these to DCHECKs? I don't > see > > a > > > > > > reason why they should be CHECKs. > > > > > > > > > > The reason is potential execution of random pieces of memory. Please do > > not > > > > > remove the checks. Please see > https://codereview.chromium.org/2728773005/ > > > for > > > > > more context. > > > > > > > > I see - you worried about slicing causing execution of random code. > > > > Would you think that it's ok to remove these CHECKs after value conversion > > is > > > > done? > > > > It has a measurable performance impact (not very big though). > > > > I don't think this is due to slicing, it is more the issue of having the union > > of storage inside base::Value. That's not going away even if base::Value will > no > > longer be inherited from. > > Well, I'm looking at your example. > > If DictionaryValue is a part of Value, I think this code: > 1: DictionaryValue d; > 2: Value *p = &d; > 3: *p = Value(1.234); > 4: DictionaryValue().Swap(&d); > > It seems to be OK, since it's just: > > Value d; > Value* p = &d; > *p = Value(1.234); > Value().Swap(&d); > > p is now an empty value, is it not? The latter code discards the issue with type_ == DICTIONARY by using an empty Value instead of Value(Type::DICTIONARY). While I don't see a way currently to end up calling InternalCopyAssignFromSameType with non-matching types once there are no subclasses to Value, that might change if somebody makes a mistake in the code later. It is not prevented by Value having no subclasses. (Just to avoid misunderstanding: I am responding to the particular questions, but I maintain my response in #17 where I already yielded to dcheng's position on the question of removing the CHECK.) Cheers, Vaclav
On 2017/03/16 08:36:49, vabr (Chromium) wrote: > On 2017/03/15 20:04:48, dyaroshev wrote: > > On 2017/03/14 08:13:46, vabr (Chromium) wrote: > > > On 2017/03/14 05:08:02, dcheng wrote: > > > > https://codereview.chromium.org/2740373002/diff/40001/base/values.cc > > > > File base/values.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2740373002/diff/40001/base/values.cc#newcode530 > > > > base/values.cc:530: CHECK_EQ(type_, that.type_); > > > > On 2017/03/13 21:15:42, dyaroshev wrote: > > > > > On 2017/03/13 10:56:01, vabr (Chromium) wrote: > > > > > > On 2017/03/11 06:30:07, dcheng wrote: > > > > > > > While you're fixing this, do mind changing these to DCHECKs? I don't > > see > > > a > > > > > > > reason why they should be CHECKs. > > > > > > > > > > > > The reason is potential execution of random pieces of memory. Please > do > > > not > > > > > > remove the checks. Please see > > https://codereview.chromium.org/2728773005/ > > > > for > > > > > > more context. > > > > > > > > > > I see - you worried about slicing causing execution of random code. > > > > > Would you think that it's ok to remove these CHECKs after value > conversion > > > is > > > > > done? > > > > > It has a measurable performance impact (not very big though). > > > > > > I don't think this is due to slicing, it is more the issue of having the > union > > > of storage inside base::Value. That's not going away even if base::Value > will > > no > > > longer be inherited from. > > > > Well, I'm looking at your example. > > > > If DictionaryValue is a part of Value, I think this code: > > 1: DictionaryValue d; > > 2: Value *p = &d; > > 3: *p = Value(1.234); > > 4: DictionaryValue().Swap(&d); > > > > It seems to be OK, since it's just: > > > > Value d; > > Value* p = &d; > > *p = Value(1.234); > > Value().Swap(&d); > > > > p is now an empty value, is it not? > > The latter code discards the issue with type_ == DICTIONARY by using an empty > Value instead of Value(Type::DICTIONARY). > > While I don't see a way currently to end up calling > InternalCopyAssignFromSameType with non-matching types once there are no > subclasses to Value, that might change if somebody makes a mistake in the code > later. It is not prevented by Value having no subclasses. > > > (Just to avoid misunderstanding: I am responding to the particular questions, > but I maintain my response in #17 where I already yielded to dcheng's position > on the question of removing the CHECK.) > > Cheers, > Vaclav FWIW, I'm OK with temporarily keeping the CHECKs if they're converted to DCHECKs once all the subclasses are collapsed into the base class and removed.
On 2017/03/16 08:39:06, dcheng wrote: > On 2017/03/16 08:36:49, vabr (Chromium) wrote: > > On 2017/03/15 20:04:48, dyaroshev wrote: > > > On 2017/03/14 08:13:46, vabr (Chromium) wrote: > > > > On 2017/03/14 05:08:02, dcheng wrote: > > > > > https://codereview.chromium.org/2740373002/diff/40001/base/values.cc > > > > > File base/values.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2740373002/diff/40001/base/values.cc#newcode530 > > > > > base/values.cc:530: CHECK_EQ(type_, that.type_); > > > > > On 2017/03/13 21:15:42, dyaroshev wrote: > > > > > > On 2017/03/13 10:56:01, vabr (Chromium) wrote: > > > > > > > On 2017/03/11 06:30:07, dcheng wrote: > > > > > > > > While you're fixing this, do mind changing these to DCHECKs? I > don't > > > see > > > > a > > > > > > > > reason why they should be CHECKs. > > > > > > > > > > > > > > The reason is potential execution of random pieces of memory. Please > > do > > > > not > > > > > > > remove the checks. Please see > > > https://codereview.chromium.org/2728773005/ > > > > > for > > > > > > > more context. > > > > > > > > > > > > I see - you worried about slicing causing execution of random code. > > > > > > Would you think that it's ok to remove these CHECKs after value > > conversion > > > > is > > > > > > done? > > > > > > It has a measurable performance impact (not very big though). > > > > > > > > I don't think this is due to slicing, it is more the issue of having the > > union > > > > of storage inside base::Value. That's not going away even if base::Value > > will > > > no > > > > longer be inherited from. > > > > > > Well, I'm looking at your example. > > > > > > If DictionaryValue is a part of Value, I think this code: > > > 1: DictionaryValue d; > > > 2: Value *p = &d; > > > 3: *p = Value(1.234); > > > 4: DictionaryValue().Swap(&d); > > > > > > It seems to be OK, since it's just: > > > > > > Value d; > > > Value* p = &d; > > > *p = Value(1.234); > > > Value().Swap(&d); > > > > > > p is now an empty value, is it not? > > > > The latter code discards the issue with type_ == DICTIONARY by using an empty > > Value instead of Value(Type::DICTIONARY). > > > > While I don't see a way currently to end up calling > > InternalCopyAssignFromSameType with non-matching types once there are no > > subclasses to Value, that might change if somebody makes a mistake in the code > > later. It is not prevented by Value having no subclasses. > > > > > > (Just to avoid misunderstanding: I am responding to the particular questions, > > but I maintain my response in #17 where I already yielded to dcheng's position > > on the question of removing the CHECK.) > > > > Cheers, > > Vaclav > > FWIW, I'm OK with temporarily keeping the CHECKs if they're converted to DCHECKs > once all the subclasses are collapsed into the base class and removed. Thanks, that sounds reasonable to me. While now this is relatively likely to be exploitable (through a mistake in a code using public API of Value), once Value is collapsed, there would need to be an error inside Value internals to make this exploitable. I agree that that decreases the added value of CHECK. Keeping a DCHECK, OTOH, increases the likelihood that developers discover a mistake inside Value if one appears. So I support dcheng@'s proposal above. We could add a TODO(crbug.com/646113) to mark the intention of changing the CHECK to a DCHECK. Apropos, dyaroshev@, could you please add BUG=646113 to the CL description? Cheers, Vaclav
On 2017/03/14 09:56:53, dyaroshev wrote: > On 2017/03/14 08:41:04, jdoerrie wrote: > > > > This again is on a 64 bit Linux with Clang, I currently don't have easy access > > to other platforms, unfortunately. > > Judging by this results there is a performance hit of about ~60% when dropping > > |InternalMoveAssignFromSameType|. > > This makes sense to me, because we always do a memory allocation in that case > > (https://cs.chromium.org/chromium/src/base/memory/manual_constructor.h?l=57). > > > > I'm not sure what you mean - this is placement new, it just calls the > constructor of std::string, move constructor of std::string should not call new. Yeah, you are right, my bad. > Oh! I - think I have an idea! > You test on linux, right? We have some issues with the standard library on linux > - it's older and does not support C++11 fully. Can you please check if the > standard library you are using has move constructors in std::string? I hope I did this correctly, but judging by the include (include/c++/4.6/bits/basic_string.h) path it seems like I am using the libstdc++ shipped with GCC 4.6. Here string does have a move constructor and a move assignment operator: https://gcc.gnu.org/onlinedocs/gcc-4.6.4/libstdc++/api/a00783_source.html#l00494 https://gcc.gnu.org/onlinedocs/gcc-4.6.4/libstdc++/api/a00783_source.html#l00566 However, you are certainly right that this implementation is very old. Do you happen to know whether there is an easy way to use a newer libstdc++ or libc++ instead? I did get my hands on a Mac Book Pro in the meantime. My findings here are that sometimes |InternalMoveAssignFromSameType| is faster, sometimes it is slower. Both implementations range between 90,000 and 150,000 us in my tests for 1M strings. Given these rather inconclusive results I don't feel strongly regarding keeping |InternalMoveAssignFromSameType| anymore and this CL LGTM as is.
On 2017/03/16 13:55:50, jdoerrie wrote: > On 2017/03/14 09:56:53, dyaroshev wrote: > > On 2017/03/14 08:41:04, jdoerrie wrote: > > > > > > This again is on a 64 bit Linux with Clang, I currently don't have easy > access > > > to other platforms, unfortunately. > > > Judging by this results there is a performance hit of about ~60% when > dropping > > > |InternalMoveAssignFromSameType|. > > > This makes sense to me, because we always do a memory allocation in that > case > > > > (https://cs.chromium.org/chromium/src/base/memory/manual_constructor.h?l=57). > > > > > > > I'm not sure what you mean - this is placement new, it just calls the > > constructor of std::string, move constructor of std::string should not call > new. > > Yeah, you are right, my bad. > > > Oh! I - think I have an idea! > > You test on linux, right? We have some issues with the standard library on > linux > > - it's older and does not support C++11 fully. Can you please check if the > > standard library you are using has move constructors in std::string? > > I hope I did this correctly, but judging by the include > (include/c++/4.6/bits/basic_string.h) path it seems like I am using the > libstdc++ shipped with GCC 4.6. Here string does have a move constructor and a > move assignment operator: > > https://gcc.gnu.org/onlinedocs/gcc-4.6.4/libstdc++/api/a00783_source.html#l00494 > https://gcc.gnu.org/onlinedocs/gcc-4.6.4/libstdc++/api/a00783_source.html#l00566 > > However, you are certainly right that this implementation is very old. Do you > happen to know whether there is an easy way to use a newer libstdc++ or libc++ > instead? > Unfortunatly as far as I know - chromium downloads it's standard library from some online storage. I have no idea why we don't update it. It would be terribly nice. > I did get my hands on a Mac Book Pro in the meantime. My findings here are that > sometimes |InternalMoveAssignFromSameType| is faster, sometimes it is slower. > Both implementations range between 90,000 and 150,000 us in my tests for 1M > strings. > Given these rather inconclusive results I don't feel strongly regarding keeping > |InternalMoveAssignFromSameType| anymore and this CL LGTM as is. To be sure I did a version of this test on windows - just removed the call to InternalAssigment. With Internal Move Assign: *RESULT StringMoveAssign: Max Length: 1, TestOrder: ascending= 9 us *RESULT StringMoveAssign: Max Length: 2, TestOrder: ascending= 1 us *RESULT StringMoveAssign: Max Length: 4, TestOrder: ascending= 1 us *RESULT StringMoveAssign: Max Length: 8, TestOrder: ascending= 2 us *RESULT StringMoveAssign: Max Length: 16, TestOrder: ascending= 2 us *RESULT StringMoveAssign: Max Length: 32, TestOrder: ascending= 3 us *RESULT StringMoveAssign: Max Length: 64, TestOrder: ascending= 7 us *RESULT StringMoveAssign: Max Length: 128, TestOrder: ascending= 10 us *RESULT StringMoveAssign: Max Length: 256, TestOrder: ascending= 33 us *RESULT StringMoveAssign: Max Length: 512, TestOrder: ascending= 92 us *RESULT StringMoveAssign: Max Length: 1024, TestOrder: ascending= 164 us *RESULT StringMoveAssign: Max Length: 2048, TestOrder: ascending= 336 us *RESULT StringMoveAssign: Max Length: 4096, TestOrder: ascending= 619 us *RESULT StringMoveAssign: Max Length: 8192, TestOrder: ascending= 1325 us *RESULT StringMoveAssign: Max Length: 16384, TestOrder: ascending= 2540 us *RESULT StringMoveAssign: Max Length: 32768, TestOrder: ascending= 5434 us *RESULT StringMoveAssign: Max Length: 65536, TestOrder: ascending= 10779 us *RESULT StringMoveAssign: Max Length: 131072, TestOrder: ascending= 22099 us *RESULT StringMoveAssign: Max Length: 262144, TestOrder: ascending= 44331 us Without: *RESULT StringMoveAssign: Max Length: 1, TestOrder: ascending= 3 us *RESULT StringMoveAssign: Max Length: 2, TestOrder: ascending= 0 us *RESULT StringMoveAssign: Max Length: 4, TestOrder: ascending= 0 us *RESULT StringMoveAssign: Max Length: 8, TestOrder: ascending= 3 us *RESULT StringMoveAssign: Max Length: 16, TestOrder: ascending= 1 us *RESULT StringMoveAssign: Max Length: 32, TestOrder: ascending= 2 us *RESULT StringMoveAssign: Max Length: 64, TestOrder: ascending= 9 us *RESULT StringMoveAssign: Max Length: 128, TestOrder: ascending= 13 us *RESULT StringMoveAssign: Max Length: 256, TestOrder: ascending= 32 us *RESULT StringMoveAssign: Max Length: 512, TestOrder: ascending= 90 us *RESULT StringMoveAssign: Max Length: 1024, TestOrder: ascending= 170 us *RESULT StringMoveAssign: Max Length: 2048, TestOrder: ascending= 577 us *RESULT StringMoveAssign: Max Length: 4096, TestOrder: ascending= 1270 us *RESULT StringMoveAssign: Max Length: 8192, TestOrder: ascending= 3393 us *RESULT StringMoveAssign: Max Length: 16384, TestOrder: ascending= 2550 us *RESULT StringMoveAssign: Max Length: 32768, TestOrder: ascending= 5425 us *RESULT StringMoveAssign: Max Length: 65536, TestOrder: ascending= 11143 us *RESULT StringMoveAssign: Max Length: 131072, TestOrder: ascending= 22237 us *RESULT StringMoveAssign: Max Length: 262144, TestOrder: ascending= 44289 us ////////////////////////// To conclude - doesn't seem to matter on Windows.
On 2017/03/16 21:02:51, dyaroshev wrote: > On 2017/03/16 13:55:50, jdoerrie wrote: > > On 2017/03/14 09:56:53, dyaroshev wrote: > > > On 2017/03/14 08:41:04, jdoerrie wrote: > > > > > > > > This again is on a 64 bit Linux with Clang, I currently don't have easy > > access > > > > to other platforms, unfortunately. > > > > Judging by this results there is a performance hit of about ~60% when > > dropping > > > > |InternalMoveAssignFromSameType|. > > > > This makes sense to me, because we always do a memory allocation in that > > case > > > > > > (https://cs.chromium.org/chromium/src/base/memory/manual_constructor.h?l=57). > > > > > > > > > > I'm not sure what you mean - this is placement new, it just calls the > > > constructor of std::string, move constructor of std::string should not call > > new. > > > > Yeah, you are right, my bad. > > > > > Oh! I - think I have an idea! > > > You test on linux, right? We have some issues with the standard library on > > linux > > > - it's older and does not support C++11 fully. Can you please check if the > > > standard library you are using has move constructors in std::string? > > > > I hope I did this correctly, but judging by the include > > (include/c++/4.6/bits/basic_string.h) path it seems like I am using the > > libstdc++ shipped with GCC 4.6. Here string does have a move constructor and a > > move assignment operator: > > > > > https://gcc.gnu.org/onlinedocs/gcc-4.6.4/libstdc++/api/a00783_source.html#l00494 > > > https://gcc.gnu.org/onlinedocs/gcc-4.6.4/libstdc++/api/a00783_source.html#l00566 > > > > However, you are certainly right that this implementation is very old. Do you > > happen to know whether there is an easy way to use a newer libstdc++ or libc++ > > instead? > > > > Unfortunatly as far as I know - chromium downloads it's standard library from > some online storage. I have no idea why we don't update it. It would be terribly > nice. I believe this is to get reproducible / hermetic builds, and we're working on upgrading it atm: https://bugs.chromium.org/p/chromium/issues/detail?id=564904 Hopefully the CL to update them will stick soon. > > > I did get my hands on a Mac Book Pro in the meantime. My findings here are > that > > sometimes |InternalMoveAssignFromSameType| is faster, sometimes it is slower. > > Both implementations range between 90,000 and 150,000 us in my tests for 1M > > strings. > > Given these rather inconclusive results I don't feel strongly regarding > keeping > > |InternalMoveAssignFromSameType| anymore and this CL LGTM as is. > > To be sure I did a version of this test on windows - just removed the call to > InternalAssigment. > > With Internal Move Assign: > > *RESULT StringMoveAssign: Max Length: 1, TestOrder: ascending= 9 us > *RESULT StringMoveAssign: Max Length: 2, TestOrder: ascending= 1 us > *RESULT StringMoveAssign: Max Length: 4, TestOrder: ascending= 1 us > *RESULT StringMoveAssign: Max Length: 8, TestOrder: ascending= 2 us > *RESULT StringMoveAssign: Max Length: 16, TestOrder: ascending= 2 us > *RESULT StringMoveAssign: Max Length: 32, TestOrder: ascending= 3 us > *RESULT StringMoveAssign: Max Length: 64, TestOrder: ascending= 7 us > *RESULT StringMoveAssign: Max Length: 128, TestOrder: ascending= 10 us > *RESULT StringMoveAssign: Max Length: 256, TestOrder: ascending= 33 us > *RESULT StringMoveAssign: Max Length: 512, TestOrder: ascending= 92 us > *RESULT StringMoveAssign: Max Length: 1024, TestOrder: ascending= 164 us > *RESULT StringMoveAssign: Max Length: 2048, TestOrder: ascending= 336 us > *RESULT StringMoveAssign: Max Length: 4096, TestOrder: ascending= 619 us > *RESULT StringMoveAssign: Max Length: 8192, TestOrder: ascending= 1325 us > *RESULT StringMoveAssign: Max Length: 16384, TestOrder: ascending= 2540 us > *RESULT StringMoveAssign: Max Length: 32768, TestOrder: ascending= 5434 us > *RESULT StringMoveAssign: Max Length: 65536, TestOrder: ascending= 10779 us > *RESULT StringMoveAssign: Max Length: 131072, TestOrder: ascending= 22099 us > *RESULT StringMoveAssign: Max Length: 262144, TestOrder: ascending= 44331 us > > Without: > > *RESULT StringMoveAssign: Max Length: 1, TestOrder: ascending= 3 us > *RESULT StringMoveAssign: Max Length: 2, TestOrder: ascending= 0 us > *RESULT StringMoveAssign: Max Length: 4, TestOrder: ascending= 0 us > *RESULT StringMoveAssign: Max Length: 8, TestOrder: ascending= 3 us > *RESULT StringMoveAssign: Max Length: 16, TestOrder: ascending= 1 us > *RESULT StringMoveAssign: Max Length: 32, TestOrder: ascending= 2 us > *RESULT StringMoveAssign: Max Length: 64, TestOrder: ascending= 9 us > *RESULT StringMoveAssign: Max Length: 128, TestOrder: ascending= 13 us > *RESULT StringMoveAssign: Max Length: 256, TestOrder: ascending= 32 us > *RESULT StringMoveAssign: Max Length: 512, TestOrder: ascending= 90 us > *RESULT StringMoveAssign: Max Length: 1024, TestOrder: ascending= 170 us > *RESULT StringMoveAssign: Max Length: 2048, TestOrder: ascending= 577 us > *RESULT StringMoveAssign: Max Length: 4096, TestOrder: ascending= 1270 us > *RESULT StringMoveAssign: Max Length: 8192, TestOrder: ascending= 3393 us > *RESULT StringMoveAssign: Max Length: 16384, TestOrder: ascending= 2550 us > *RESULT StringMoveAssign: Max Length: 32768, TestOrder: ascending= 5425 us > *RESULT StringMoveAssign: Max Length: 65536, TestOrder: ascending= 11143 us > *RESULT StringMoveAssign: Max Length: 131072, TestOrder: ascending= 22237 us > *RESULT StringMoveAssign: Max Length: 262144, TestOrder: ascending= 44289 us > > ////////////////////////// > > To conclude - doesn't seem to matter on Windows.
On 2017/03/16 21:11:33, dcheng wrote: > On 2017/03/16 21:02:51, dyaroshev wrote: > > On 2017/03/16 13:55:50, jdoerrie wrote: > > > However, you are certainly right that this implementation is very old. Do > you > > > happen to know whether there is an easy way to use a newer libstdc++ or > libc++ > > > instead? > > > > > > > Unfortunatly as far as I know - chromium downloads it's standard library from > > some online storage. I have no idea why we don't update it. It would be > terribly > > nice. > > I believe this is to get reproducible / hermetic builds, and we're working on > upgrading it atm: https://bugs.chromium.org/p/chromium/issues/detail?id=564904 > > Hopefully the CL to update them will stick soon. > I don't have access. But it's great to hear.
Description was changed from ========== Removing checks for self-assignment from base::Value. Discussion on self-assignment: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/ZzfGfgruHxc ========== to ========== Removing checks for self-assignment from base::Value. Discussion on self-assignment: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/ZzfGfgruHxc BUG=646113 ==========
Description was changed from ========== Removing checks for self-assignment from base::Value. Discussion on self-assignment: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/ZzfGfgruHxc BUG=646113 ========== to ========== Removing checks for self-assignment from base::Value. Discussion on self-assignment: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/ZzfGfgruHxc Always destroying and creating instead of InternalMoveAssign. BUG=646113 ==========
Description was changed from ========== Removing checks for self-assignment from base::Value. Discussion on self-assignment: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/ZzfGfgruHxc Always destroying and creating instead of InternalMoveAssign. BUG=646113 ========== to ========== 1 - Removing checks for self-assignment from base::Value. Discussion on self-assignment: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/ZzfGfgruHxc 2 - Always destroying and creating instead of InternalMoveAssign. BUG=646113 ==========
The CQ bit was checked by dyaroshev@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, jdoerrie@chromium.org Link to the patchset: https://codereview.chromium.org/2740373002/#ps80001 (title: "Review, round 2.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for base/values.cc:
While running git apply --index -p1;
error: patch failed: base/values.cc:562
error: base/values.cc: patch does not apply
Patch: base/values.cc
Index: base/values.cc
diff --git a/base/values.cc b/base/values.cc
index
5cc0d693bdd11c12e60f14f8921bec3cd303e4da..fef999c43b537619d9e5ccc0c1ec966e5e52ae79
100644
--- a/base/values.cc
+++ b/base/values.cc
@@ -173,27 +173,21 @@ Value::Value(std::vector<char>&& in_blob) :
type_(Type::BINARY) {
}
Value& Value::operator=(const Value& that) {
- if (this != &that) {
- if (type_ == that.type_) {
- InternalCopyAssignFromSameType(that);
- } else {
- InternalCleanup();
- InternalCopyConstructFrom(that);
- }
+ if (type_ == that.type_) {
+ InternalCopyAssignFromSameType(that);
+ } else {
+ // This is not a self assignment because the type_ doesn't match.
+ InternalCleanup();
+ InternalCopyConstructFrom(that);
}
return *this;
}
Value& Value::operator=(Value&& that) {
- if (this != &that) {
- if (type_ == that.type_) {
- InternalMoveAssignFromSameType(std::move(that));
- } else {
- InternalCleanup();
- InternalMoveConstructFrom(std::move(that));
- }
- }
+ DCHECK(this != &that) << "attempt to self move assign.";
+ InternalCleanup();
+ InternalMoveConstructFrom(std::move(that));
return *this;
}
@@ -533,6 +527,8 @@ void Value::InternalMoveConstructFrom(Value&& that) {
}
void Value::InternalCopyAssignFromSameType(const Value& that) {
+ // TODO(crbug.com/646113): make this a DCHECK once base::Value does not have
+ // subclasses.
CHECK_EQ(type_, that.type_);
switch (type_) {
@@ -562,32 +558,6 @@ void Value::InternalCopyAssignFromSameType(const Value&
that) {
}
}
-void Value::InternalMoveAssignFromSameType(Value&& that) {
- CHECK_EQ(type_, that.type_);
-
- switch (type_) {
- case Type::NONE:
- case Type::BOOLEAN:
- case Type::INTEGER:
- case Type::DOUBLE:
- InternalCopyFundamentalValue(that);
- return;
-
- case Type::STRING:
- *string_value_ = std::move(*that.string_value_);
- return;
- case Type::BINARY:
- *binary_value_ = std::move(*that.binary_value_);
- return;
- case Type::DICTIONARY:
- *dict_ptr_ = std::move(*that.dict_ptr_);
- return;
- case Type::LIST:
- *list_ = std::move(*that.list_);
- return;
- }
-}
-
void Value::InternalCleanup() {
switch (type_) {
case Type::NONE:
The CQ bit was checked by dyaroshev@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from jdoerrie@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2740373002/#ps100001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dyaroshev@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490021027875880,
"parent_rev": "fa0844ccdce295f9789664668f358d2df1a69b09", "commit_rev":
"534a0fda6258fcff015ea6a95f9fba981596c07e"}
Message was sent while issue was closed.
Description was changed from ========== 1 - Removing checks for self-assignment from base::Value. Discussion on self-assignment: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/ZzfGfgruHxc 2 - Always destroying and creating instead of InternalMoveAssign. BUG=646113 ========== to ========== 1 - Removing checks for self-assignment from base::Value. Discussion on self-assignment: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/ZzfGfgruHxc 2 - Always destroying and creating instead of InternalMoveAssign. BUG=646113 Review-Url: https://codereview.chromium.org/2740373002 Cr-Commit-Position: refs/heads/master@{#458101} Committed: https://chromium.googlesource.com/chromium/src/+/534a0fda6258fcff015ea6a95f9f... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/534a0fda6258fcff015ea6a95f9f... |
