Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2837873007:
Tell V8 about the extra memory being held by FileReader objects. (Closed)
DescriptionTell V8 about the extra memory being held by FileReader objects.
BUG=114548
Review-Url: https://codereview.chromium.org/2837873007
Cr-Commit-Position: refs/heads/master@{#468550}
Committed: https://chromium.googlesource.com/chromium/src/+/93af0c350008bf6e45ca995a8d88d161c8a384e0
Patch Set 1 #
Total comments: 1
Patch Set 2 : rebase #
Total comments: 2
Patch Set 3 : more accurate #
Total comments: 8
Patch Set 4 : comments #
Messages
Total messages: 35 (21 generated)
The CQ bit was checked by michaeln@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...
michaeln@chromium.org changed reviewers: + dmurph@chromium.org
https://codereview.chromium.org/2837873007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp (right): https://codereview.chromium.org/2837873007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp:353: ReportAdditionalMemoryUsageToV8(string_result_.CharactersSizeInBytes()); Hmmm... I think this can miss reporting the the string result in some cases where the result is sample prior to load completion. I'm not sure how much accuracy matters though? We could dbl count the raw data bytes appended and not even try to count the string_result_ independently. That would error on the side of over-reporting rather than under-reporting which might be a better compromise. Assuming somewhat over-reporting is not a problem, I think I'd prefer that to a bunch of code to perform careful accounting for the string_result_. wdyt?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by michaeln@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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
WDYT? https://codereview.chromium.org/2837873007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp (right): https://codereview.chromium.org/2837873007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp:352: if (finished_loading_) To make this more clear, can you have the two Convert methods return the string, which you then std::move into string_result_? Then it makes more sense in this method that you're allocating that memory right now, and you want to report it. Then, if we're finished loading: * clear raw_data_. * int64_t size_diff = string_result_.size() - memory_usage_reported_to_v8_; AdjustAdditionalV8MemoryUsage(size_diff). That makes the memory amount accurate, and we clear raw_data_ (which it looks like we're hanging on to for some reason).
https://codereview.chromium.org/2837873007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp (right): https://codereview.chromium.org/2837873007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp:352: if (finished_loading_) ok, i made it more accurate and thnx for pointing out that we don't need to hang onto raw_data_ beyond this point https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fileapi/FileReaderLoader.h (right): https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fileapi/FileReaderLoader.h:150: int64_t memory_usage_reported_to_v8_ = 0; i can switch more of the data members to initialize here too
lgtm with nits https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp (right): https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp:156: memory_usage_reported_to_v8_ += usage; do a bounds check so it doesn't go negative (dcheck maybe) https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp:318: ReportAdditionalMemoryUsageToV8(-1 * raw_data_->ByteLength()); change to AdjustReportedV8MemoryUsage? https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp:339: SetStringResult(ConvertToText()); Do you need a std::move here?
https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fileapi/FileReaderLoader.h (right): https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fileapi/FileReaderLoader.h:150: int64_t memory_usage_reported_to_v8_ = 0; On 2017/04/28 00:54:25, michaeln wrote: > i can switch more of the data members to initialize here too That would be great actually. I like that style.
thnx https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp (right): https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp:156: memory_usage_reported_to_v8_ += usage; On 2017/04/28 02:33:48, dmurph wrote: > do a bounds check so it doesn't go negative (dcheck maybe) Done. https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp:318: ReportAdditionalMemoryUsageToV8(-1 * raw_data_->ByteLength()); On 2017/04/28 02:33:48, dmurph wrote: > change to AdjustReportedV8MemoryUsage? Done. https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp:339: SetStringResult(ConvertToText()); On 2017/04/28 02:33:48, dmurph wrote: > Do you need a std::move here? I don't think that would do anything, SetStringResult takes a const ref as input.
The CQ bit was checked by michaeln@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmurph@chromium.org Link to the patchset: https://codereview.chromium.org/2837873007/#ps60001 (title: "comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/28 23:08:51, michaeln wrote: > thnx > > https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp (right): > > https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp:156: > memory_usage_reported_to_v8_ += usage; > On 2017/04/28 02:33:48, dmurph wrote: > > do a bounds check so it doesn't go negative (dcheck maybe) > > Done. > > https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp:318: > ReportAdditionalMemoryUsageToV8(-1 * raw_data_->ByteLength()); > On 2017/04/28 02:33:48, dmurph wrote: > > change to AdjustReportedV8MemoryUsage? > > Done. > > https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp:339: > SetStringResult(ConvertToText()); > On 2017/04/28 02:33:48, dmurph wrote: > > Do you need a std::move here? > > I don't think that would do anything, SetStringResult takes a const ref as > input. Can you use a different method? Or can you just std::move directly into string_result_? So we don't have to do a double allocation here?
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2017/04/28 23:10:43, dmurph wrote: > On 2017/04/28 23:08:51, michaeln wrote: > > thnx > > > > > https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp (right): > > > > > https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp:156: > > memory_usage_reported_to_v8_ += usage; > > On 2017/04/28 02:33:48, dmurph wrote: > > > do a bounds check so it doesn't go negative (dcheck maybe) > > > > Done. > > > > > https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp:318: > > ReportAdditionalMemoryUsageToV8(-1 * raw_data_->ByteLength()); > > On 2017/04/28 02:33:48, dmurph wrote: > > > change to AdjustReportedV8MemoryUsage? > > > > Done. > > > > > https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp:339: > > SetStringResult(ConvertToText()); > > On 2017/04/28 02:33:48, dmurph wrote: > > > Do you need a std::move here? > > > > I don't think that would do anything, SetStringResult takes a const ref as > > input. > > Can you use a different method? Or can you just std::move directly into > string_result_? So we don't have to do a double allocation here? like we talked about offline... there is no copy of the data being made here... a ref is added str.impl_ is all
Description was changed from ========== Tell V8 about the extra memory being held by FileReader objects. BUG=114548 ========== to ========== Tell V8 about the extra memory being held by FileReader objects. BUG=114548 ==========
michaeln@chromium.org changed reviewers: + kinuko@chromium.org
michaeln@chromium.org changed reviewers: - kinuko@chromium.org
michaeln@chromium.org changed reviewers: + japhet@chromium.org
michaeln@chromium.org changed reviewers: + kinuko@chromium.org - japhet@chromium.org
@kinuko, for an OWNERS review, thnx
lgtm
The CQ bit was checked by michaeln@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": 60001, "attempt_start_ts": 1493688301986200,
"parent_rev": "59df29deed59592d42ff7e600f33faf035e00b46", "commit_rev":
"7af867cad674a8650c2e1e7ca56f814f91eaaac5"}
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493688301986200,
"parent_rev": "b87a74749197d2ca9a718f2fe4d230c950d98b72", "commit_rev":
"93af0c350008bf6e45ca995a8d88d161c8a384e0"}
Message was sent while issue was closed.
Description was changed from ========== Tell V8 about the extra memory being held by FileReader objects. BUG=114548 ========== to ========== Tell V8 about the extra memory being held by FileReader objects. BUG=114548 Review-Url: https://codereview.chromium.org/2837873007 Cr-Commit-Position: refs/heads/master@{#468550} Committed: https://chromium.googlesource.com/chromium/src/+/93af0c350008bf6e45ca995a8d88... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/93af0c350008bf6e45ca995a8d88... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
