Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2556773002:
[regexp] Shrink results array in @@match and @@split (Closed)
Description[regexp] Shrink results array in @@match and @@split
Both @@match and @@split internally use dynamically growing fixed
arrays. Shrink to fit when wrapping these in a JSArray to avoid
excessive memory usage.
BUG=chromium:670205, chromium:670708
Committed: https://crrev.com/40e176056d9640635d3bbc9f4e63b50ccc9057bc
Cr-Commit-Position: refs/heads/master@{#41550}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Address comments #Messages
Total messages: 25 (16 generated)
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
jgruber@chromium.org changed reviewers: + yangguo@chromium.org
Description was changed from ========== [regexp] Shrink results array in @@match and @@split Both @@match and @@split internally use dynamically growing fixed arrays. Shrink to fit when wrapping these in a JSArray to avoid excessive memory usage. BUG=chromium:670205 ========== to ========== [regexp] Shrink results array in @@match and @@split Both @@match and @@split internally use dynamically growing fixed arrays. Shrink to fit when wrapping these in a JSArray to avoid excessive memory usage. BUG=chromium:670205,chromium:670708 ==========
ishell@chromium.org changed reviewers: + ishell@chromium.org
lgtm https://codereview.chromium.org/2556773002/diff/1/src/builtins/builtins-regex... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2556773002/diff/1/src/builtins/builtins-regex... src/builtins/builtins-regexp.cc:1475: Node* ResizeFixedArray(Node* const element_count, Node* const new_capacity, Maybe CopyResizedFixedArray() would be a better name? Please add a comment that this function always does the copying and can not shrink.
https://codereview.chromium.org/2556773002/diff/1/src/builtins/builtins-regex... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2556773002/diff/1/src/builtins/builtins-regex... src/builtins/builtins-regexp.cc:1475: Node* ResizeFixedArray(Node* const element_count, Node* const new_capacity, On 2016/12/07 08:14:27, Igor Sheludko wrote: > Maybe CopyResizedFixedArray() would be a better name? Please add a comment that > this function always does the copying and can not shrink. Hmm, what do you mean by 'cannot shrink'? It can and does shrink the array (if we understand 'create new, smaller array and copy elements' as shrinking). Personally I think that 'ResizeFixedArray' is the better name, it's more clear to me what it does than with 'CopyResizedFixedArray'. I can change it though if you feel strongly about it. Will add a comment.
https://codereview.chromium.org/2556773002/diff/1/src/builtins/builtins-regex... File src/builtins/builtins-regexp.cc (right): https://codereview.chromium.org/2556773002/diff/1/src/builtins/builtins-regex... src/builtins/builtins-regexp.cc:1475: Node* ResizeFixedArray(Node* const element_count, Node* const new_capacity, On 2016/12/07 08:26:48, jgruber wrote: > On 2016/12/07 08:14:27, Igor Sheludko wrote: > > Maybe CopyResizedFixedArray() would be a better name? Please add a comment > that > > this function always does the copying and can not shrink. > > Hmm, what do you mean by 'cannot shrink'? It can and does shrink the array (if > we understand 'create new, smaller array and copy elements' as shrinking). > > Personally I think that 'ResizeFixedArray' is the better name, it's more clear > to me what it does than with 'CopyResizedFixedArray'. I can change it though if > you feel strongly about it. > > Will add a comment. If you still prefer your name, then I'm fine. I just wanted to make it visible somehow that this function does not trim the array. Please ignore my comment about shrinking, my bad.
On 2016/12/07 08:37:25, Igor Sheludko wrote: > https://codereview.chromium.org/2556773002/diff/1/src/builtins/builtins-regex... > File src/builtins/builtins-regexp.cc (right): > > https://codereview.chromium.org/2556773002/diff/1/src/builtins/builtins-regex... > src/builtins/builtins-regexp.cc:1475: Node* ResizeFixedArray(Node* const > element_count, Node* const new_capacity, > On 2016/12/07 08:26:48, jgruber wrote: > > On 2016/12/07 08:14:27, Igor Sheludko wrote: > > > Maybe CopyResizedFixedArray() would be a better name? Please add a comment > > that > > > this function always does the copying and can not shrink. > > > > Hmm, what do you mean by 'cannot shrink'? It can and does shrink the array (if > > we understand 'create new, smaller array and copy elements' as shrinking). > > > > Personally I think that 'ResizeFixedArray' is the better name, it's more clear > > to me what it does than with 'CopyResizedFixedArray'. I can change it though > if > > you feel strongly about it. > > > > Will add a comment. > > If you still prefer your name, then I'm fine. I just wanted to make it visible > somehow that this function does not trim the array. > > Please ignore my comment about shrinking, my bad. lgtm
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by jgruber@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org, yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2556773002/#ps20001 (title: "Address comments")
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 jgruber@chromium.org
The CQ bit was checked by jgruber@chromium.org
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": 20001, "attempt_start_ts": 1481115813377990,
"parent_rev": "a59ce7e70bdb858fc4fcee50e2f196c4f96b446b", "commit_rev":
"1948f6d9a5a9a8cec9489d15e71748572b6908f2"}
Message was sent while issue was closed.
Description was changed from ========== [regexp] Shrink results array in @@match and @@split Both @@match and @@split internally use dynamically growing fixed arrays. Shrink to fit when wrapping these in a JSArray to avoid excessive memory usage. BUG=chromium:670205,chromium:670708 ========== to ========== [regexp] Shrink results array in @@match and @@split Both @@match and @@split internally use dynamically growing fixed arrays. Shrink to fit when wrapping these in a JSArray to avoid excessive memory usage. BUG=chromium:670205,chromium:670708 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [regexp] Shrink results array in @@match and @@split Both @@match and @@split internally use dynamically growing fixed arrays. Shrink to fit when wrapping these in a JSArray to avoid excessive memory usage. BUG=chromium:670205,chromium:670708 ========== to ========== [regexp] Shrink results array in @@match and @@split Both @@match and @@split internally use dynamically growing fixed arrays. Shrink to fit when wrapping these in a JSArray to avoid excessive memory usage. BUG=chromium:670205,chromium:670708 Committed: https://crrev.com/40e176056d9640635d3bbc9f4e63b50ccc9057bc Cr-Commit-Position: refs/heads/master@{#41550} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/40e176056d9640635d3bbc9f4e63b50ccc9057bc Cr-Commit-Position: refs/heads/master@{#41550} |
