Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2046933002:
[V8] Make String::CanMakeExternal ignore the length of new strings (Closed)
DescriptionMake String::CanMakeExternal ignore the length of new strings.
It is expected that temporarily used strings die while they are
in new heap. So we can avoid to pay a heavy cost to externalize
them. If they are used for times, externalization will happen
when they move to an old heap.
BUG=chrmoium:606093
Committed: https://crrev.com/7a3150d13def19b5b4e0449fefa159b4141a1470
Cr-Commit-Position: refs/heads/master@{#36907}
Patch Set 1 : #Patch Set 2 : Ignore the length of strings #
Total comments: 2
Patch Set 3 : Check IsExternal first #Patch Set 4 : Remake IsOldString #Patch Set 5 : Remove check of length #Messages
Total messages: 24 (10 generated)
Patchset #1 (id:1) has been deleted
Description was changed from
==========
Extend String::CanMakeExternal to be ignorable size of strings
BUG=chrmoium:606093
Cr-Commit-Position: refs/heads/5.3.152@{#1}
Cr-Branched-From:
9b606523b5e210f60c4260ee5f50a54e8f07adbd-refs/heads/master@{#36766}
==========
to
==========
Make String::CanMakeExternal to be able to skip externalization for all young
strings.
BUG=chrmoium:606093
==========
Description was changed from ========== Make String::CanMakeExternal to be able to skip externalization for all young strings. BUG=chrmoium:606093 ========== to ========== Make String::CanMakeExternal to be able to skip to check the length. BUG=chrmoium:606093 ==========
Description was changed from ========== Make String::CanMakeExternal to be able to skip to check the length. BUG=chrmoium:606093 ========== to ========== Make String::CanMakeExternal to be able to skip checking the length of the string. BUG=chrmoium:606093 ==========
peria@chromium.org changed reviewers: + haraken@chromium.org, jochen@chromium.org
PTL. It seems difficult to skip the externalization with only Blink-side changes, because we don't know a string is in new-heap or old-heap. A public API String::CanMakeExternal() checks where the string is in, but it also checks the length which we want to skip. So I think we need a change of this API.
jochen@chromium.org changed reviewers: + yangguo@chromium.org
what about changing CanMakeExternal to ignore the length for newspace strings? (in general, please document API methods, use_this_style for variables, and avoid overloaded methods)
Description was changed from ========== Make String::CanMakeExternal to be able to skip checking the length of the string. BUG=chrmoium:606093 ========== to ========== Make String::CanMakeExternal ignore the length of new strings. It is expected that temporarily used strings die while they are in new heap. So we can avoid to pay a heavy cost to externalize them. If they are used for times, externalization will happen when they move to an old heap. BUG=chrmoium:606093 ==========
On 2016/06/08 14:57:59, jochen wrote: > what about changing CanMakeExternal to ignore the length for newspace strings? sounds good. > (in general, please document API methods, use_this_style for variables, and > avoid overloaded methods) Oops, I'm sorry not seeing around.
I added a comment here: https://bugs.chromium.org/p/chromium/issues/detail?id=606093#c24 https://codereview.chromium.org/2046933002/diff/40001/src/api.cc File src/api.cc (left): https://codereview.chromium.org/2046933002/diff/40001/src/api.cc#oldcode6019 src/api.cc:6019: return !shape.IsExternal(); Why drop this condition? If the string is already external, we would copy out the content, and call MakeExternal only for it to return false because there is nothing to be done.
https://codereview.chromium.org/2046933002/diff/40001/src/api.cc File src/api.cc (left): https://codereview.chromium.org/2046933002/diff/40001/src/api.cc#oldcode6019 src/api.cc:6019: return !shape.IsExternal(); On 2016/06/09 05:45:05, Yang wrote: > Why drop this condition? If the string is already external, we would copy out > the content, and call MakeExternal only for it to return false because there is > nothing to be done. I agree that externalized strings should not be externalized again. However, if we keep it as-is, every string is externalized. I think it should be placed just after getting |obj|, if creating a StringShape and IsExternal() are cheaper than NewSpace::Contains(). (BTW, why don't we use Value::IsExternal() here?)
On 2016/06/09 06:51:59, peria wrote: > https://codereview.chromium.org/2046933002/diff/40001/src/api.cc > File src/api.cc (left): > > https://codereview.chromium.org/2046933002/diff/40001/src/api.cc#oldcode6019 > src/api.cc:6019: return !shape.IsExternal(); > On 2016/06/09 05:45:05, Yang wrote: > > Why drop this condition? If the string is already external, we would copy out > > the content, and call MakeExternal only for it to return false because there > is > > nothing to be done. > > I agree that externalized strings should not be externalized again. However, if > we keep it as-is, every string is externalized. > > I think it should be placed just after getting |obj|, if creating a StringShape > and IsExternal() are cheaper than NewSpace::Contains(). > (BTW, why don't we use Value::IsExternal() here? Checking for external string is probably cheaper. I also don't think we should keep using StringShape. Just use string->IsExternalString().
On 2016/06/09 07:13:43, Yang wrote: > On 2016/06/09 06:51:59, peria wrote: > > https://codereview.chromium.org/2046933002/diff/40001/src/api.cc > > File src/api.cc (left): > > > > https://codereview.chromium.org/2046933002/diff/40001/src/api.cc#oldcode6019 > > src/api.cc:6019: return !shape.IsExternal(); > > On 2016/06/09 05:45:05, Yang wrote: > > > Why drop this condition? If the string is already external, we would copy > out > > > the content, and call MakeExternal only for it to return false because there > > is > > > nothing to be done. > > > > I agree that externalized strings should not be externalized again. However, > if > > we keep it as-is, every string is externalized. > > > > I think it should be placed just after getting |obj|, if creating a > StringShape > > and IsExternal() are cheaper than NewSpace::Contains(). > > (BTW, why don't we use Value::IsExternal() here? > > Checking for external string is probably cheaper. I also don't think we should > keep using StringShape. Just use string->IsExternalString(). I made another method to check only generation. Moving IsExternal() will be done in another change.
On 2016/06/09 09:44:20, peria wrote: > On 2016/06/09 07:13:43, Yang wrote: > > On 2016/06/09 06:51:59, peria wrote: > > > https://codereview.chromium.org/2046933002/diff/40001/src/api.cc > > > File src/api.cc (left): > > > > > > https://codereview.chromium.org/2046933002/diff/40001/src/api.cc#oldcode6019 > > > src/api.cc:6019: return !shape.IsExternal(); > > > On 2016/06/09 05:45:05, Yang wrote: > > > > Why drop this condition? If the string is already external, we would copy > > out > > > > the content, and call MakeExternal only for it to return false because > there > > > is > > > > nothing to be done. > > > > > > I agree that externalized strings should not be externalized again. > However, > > if > > > we keep it as-is, every string is externalized. > > > > > > I think it should be placed just after getting |obj|, if creating a > > StringShape > > > and IsExternal() are cheaper than NewSpace::Contains(). > > > (BTW, why don't we use Value::IsExternal() here? > > > > Checking for external string is probably cheaper. I also don't think we should > > keep using StringShape. Just use string->IsExternalString(). > > I made another method to check only generation. > Moving IsExternal() will be done in another change. I'm confused... why do you want to expose an API that gives insight into V8's implementation details? Can't we simply remove the kShort check, which is what the title of this CL suggests?
On 2016/06/09 09:53:33, Yang wrote: > On 2016/06/09 09:44:20, peria wrote: > > On 2016/06/09 07:13:43, Yang wrote: > > > On 2016/06/09 06:51:59, peria wrote: > > > > https://codereview.chromium.org/2046933002/diff/40001/src/api.cc > > > > File src/api.cc (left): > > > > > > > > > https://codereview.chromium.org/2046933002/diff/40001/src/api.cc#oldcode6019 > > > > src/api.cc:6019: return !shape.IsExternal(); > > > > On 2016/06/09 05:45:05, Yang wrote: > > > > > Why drop this condition? If the string is already external, we would > copy > > > out > > > > > the content, and call MakeExternal only for it to return false because > > there > > > > is > > > > > nothing to be done. > > > > > > > > I agree that externalized strings should not be externalized again. > > However, > > > if > > > > we keep it as-is, every string is externalized. > > > > > > > > I think it should be placed just after getting |obj|, if creating a > > > StringShape > > > > and IsExternal() are cheaper than NewSpace::Contains(). > > > > (BTW, why don't we use Value::IsExternal() here? > > > > > > Checking for external string is probably cheaper. I also don't think we > should > > > keep using StringShape. Just use string->IsExternalString(). > > > > I made another method to check only generation. > > Moving IsExternal() will be done in another change. > > I'm confused... why do you want to expose an API that gives insight into V8's > implementation details? Can't we simply remove the kShort check, which is what > the title of this CL suggests? I'm sorry to be confusing. You mean the change like PS3, right? Why I'm trying to make a new API is to try using it in a specific part. If it is acceptable for Blink side to do the optimization in general, I'm happy to revert to PS3 to be reviewed.
On 2016/06/09 10:59:55, peria wrote: > On 2016/06/09 09:53:33, Yang wrote: > > On 2016/06/09 09:44:20, peria wrote: > > > On 2016/06/09 07:13:43, Yang wrote: > > > > On 2016/06/09 06:51:59, peria wrote: > > > > > https://codereview.chromium.org/2046933002/diff/40001/src/api.cc > > > > > File src/api.cc (left): > > > > > > > > > > > > https://codereview.chromium.org/2046933002/diff/40001/src/api.cc#oldcode6019 > > > > > src/api.cc:6019: return !shape.IsExternal(); > > > > > On 2016/06/09 05:45:05, Yang wrote: > > > > > > Why drop this condition? If the string is already external, we would > > copy > > > > out > > > > > > the content, and call MakeExternal only for it to return false because > > > there > > > > > is > > > > > > nothing to be done. > > > > > > > > > > I agree that externalized strings should not be externalized again. > > > However, > > > > if > > > > > we keep it as-is, every string is externalized. > > > > > > > > > > I think it should be placed just after getting |obj|, if creating a > > > > StringShape > > > > > and IsExternal() are cheaper than NewSpace::Contains(). > > > > > (BTW, why don't we use Value::IsExternal() here? > > > > > > > > Checking for external string is probably cheaper. I also don't think we > > should > > > > keep using StringShape. Just use string->IsExternalString(). > > > > > > I made another method to check only generation. > > > Moving IsExternal() will be done in another change. > > > > I'm confused... why do you want to expose an API that gives insight into V8's > > implementation details? Can't we simply remove the kShort check, which is what > > the title of this CL suggests? > > I'm sorry to be confusing. > You mean the change like PS3, right? > > Why I'm trying to make a new API is to try using it in a specific part. > If it is acceptable for Blink side to do the optimization in general, > I'm happy to revert to PS3 to be reviewed. updated to be as description says.
On 2016/06/10 07:45:27, peria wrote: > On 2016/06/09 10:59:55, peria wrote: > > On 2016/06/09 09:53:33, Yang wrote: > > > On 2016/06/09 09:44:20, peria wrote: > > > > On 2016/06/09 07:13:43, Yang wrote: > > > > > On 2016/06/09 06:51:59, peria wrote: > > > > > > https://codereview.chromium.org/2046933002/diff/40001/src/api.cc > > > > > > File src/api.cc (left): > > > > > > > > > > > > > > > https://codereview.chromium.org/2046933002/diff/40001/src/api.cc#oldcode6019 > > > > > > src/api.cc:6019: return !shape.IsExternal(); > > > > > > On 2016/06/09 05:45:05, Yang wrote: > > > > > > > Why drop this condition? If the string is already external, we would > > > copy > > > > > out > > > > > > > the content, and call MakeExternal only for it to return false > because > > > > there > > > > > > is > > > > > > > nothing to be done. > > > > > > > > > > > > I agree that externalized strings should not be externalized again. > > > > However, > > > > > if > > > > > > we keep it as-is, every string is externalized. > > > > > > > > > > > > I think it should be placed just after getting |obj|, if creating a > > > > > StringShape > > > > > > and IsExternal() are cheaper than NewSpace::Contains(). > > > > > > (BTW, why don't we use Value::IsExternal() here? > > > > > > > > > > Checking for external string is probably cheaper. I also don't think we > > > should > > > > > keep using StringShape. Just use string->IsExternalString(). > > > > > > > > I made another method to check only generation. > > > > Moving IsExternal() will be done in another change. > > > > > > I'm confused... why do you want to expose an API that gives insight into > V8's > > > implementation details? Can't we simply remove the kShort check, which is > what > > > the title of this CL suggests? > > > > I'm sorry to be confusing. > > You mean the change like PS3, right? > > > > Why I'm trying to make a new API is to try using it in a specific part. > > If it is acceptable for Blink side to do the optimization in general, > > I'm happy to revert to PS3 to be reviewed. > > updated to be as description says. Patch set 5 LGTM.
The CQ bit was checked by peria@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2046933002/100001
Message was sent while issue was closed.
Description was changed from ========== Make String::CanMakeExternal ignore the length of new strings. It is expected that temporarily used strings die while they are in new heap. So we can avoid to pay a heavy cost to externalize them. If they are used for times, externalization will happen when they move to an old heap. BUG=chrmoium:606093 ========== to ========== Make String::CanMakeExternal ignore the length of new strings. It is expected that temporarily used strings die while they are in new heap. So we can avoid to pay a heavy cost to externalize them. If they are used for times, externalization will happen when they move to an old heap. BUG=chrmoium:606093 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Make String::CanMakeExternal ignore the length of new strings. It is expected that temporarily used strings die while they are in new heap. So we can avoid to pay a heavy cost to externalize them. If they are used for times, externalization will happen when they move to an old heap. BUG=chrmoium:606093 ========== to ========== Make String::CanMakeExternal ignore the length of new strings. It is expected that temporarily used strings die while they are in new heap. So we can avoid to pay a heavy cost to externalize them. If they are used for times, externalization will happen when they move to an old heap. BUG=chrmoium:606093 Committed: https://crrev.com/7a3150d13def19b5b4e0449fefa159b4141a1470 Cr-Commit-Position: refs/heads/master@{#36907} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7a3150d13def19b5b4e0449fefa159b4141a1470 Cr-Commit-Position: refs/heads/master@{#36907} |
