Tell V8 about the extra memory being held by FileReader objects. (issue 2837873007 by michaeln@chromium.org)

[mich...@chromium.org]

Reviewers: dmurph
CL: https://codereview.chromium.org/2837873007/


https://codereview.chromium.org/2837873007/diff/1/third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp
File third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp
(right):

https://codereview.chromium.org/2837873007/diff/1/third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp#newcode353
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?

Description:
Tell V8 about the extra memory being held by FileReader objects.

BUG=114548

Affected files (+23, -0 lines):
M third_party/WebKit/Source/core/fileapi/FileReaderLoader.h
M third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp


Index: third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp
diff --git a/third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp b/third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp
index d0e1b24c2d903d35d7c45a57fef9bfe7281ea8a8..653ccd9cc0cbd35c8929a76182abf8fe53460e4a 100644
--- a/third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp
+++ b/third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp
@@ -50,6 +50,7 @@
#include "platform/wtf/text/Base64.h"
#include "platform/wtf/text/StringBuilder.h"
#include "public/platform/WebURLRequest.h"
+#include "v8/include/v8.h"

namespace blink {

@@ -69,6 +70,7 @@ FileReaderLoader::FileReaderLoader(ReadType read_type,

FileReaderLoader::~FileReaderLoader() {
Cleanup();
+ UnreportMemoryUsageToV8();
if (!url_for_reading_.IsEmpty()) {
BlobRegistry::RevokePublicBlobURL(url_for_reading_);
}
@@ -143,9 +145,23 @@ void FileReaderLoader::Cleanup() {
string_result_ = "";
is_raw_data_converted_ = true;
decoder_.reset();
+ UnreportMemoryUsageToV8();
}
}

+void FileReaderLoader::ReportAdditionalMemoryUsageToV8(int64_t usage) {
+ memory_usage_reported_to_v8_ += usage;
+ v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(usage);
+}
+
+void FileReaderLoader::UnreportMemoryUsageToV8() {
+ if (!memory_usage_reported_to_v8_)
+ return;
+ v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(
+ -memory_usage_reported_to_v8_);
+ memory_usage_reported_to_v8_ = 0;
+}
+
void FileReaderLoader::DidReceiveResponse(
unsigned long,
const ResourceResponse& response,
@@ -230,6 +246,7 @@ void FileReaderLoader::DidReceiveData(const char* data, unsigned data_length) {
}
bytes_loaded_ += bytes_appended;
is_raw_data_converted_ = false;
+ ReportAdditionalMemoryUsageToV8(bytes_appended);

if (client_)
client_->DidReceiveData();
@@ -332,6 +349,8 @@ String FileReaderLoader::StringResult() {
NOTREACHED();
}

+ if (finished_loading_)
+ ReportAdditionalMemoryUsageToV8(string_result_.CharactersSizeInBytes());
return string_result_;
}

Index: third_party/WebKit/Source/core/fileapi/FileReaderLoader.h
diff --git a/third_party/WebKit/Source/core/fileapi/FileReaderLoader.h b/third_party/WebKit/Source/core/fileapi/FileReaderLoader.h
index 3bccac7eeacccfcaf26395af2c450ba5a282da33..a481f5b96751c602e332acbcdc2ea8a809d5cf7d 100644
--- a/third_party/WebKit/Source/core/fileapi/FileReaderLoader.h
+++ b/third_party/WebKit/Source/core/fileapi/FileReaderLoader.h
@@ -112,6 +112,8 @@ class CORE_EXPORT FileReaderLoader final : public ThreadableLoaderClient {
FileReaderLoader(ReadType, FileReaderLoaderClient*);

void Cleanup();
+ void ReportAdditionalMemoryUsageToV8(int64_t usage);
+ void UnreportMemoryUsageToV8();

void Failed(FileError::ErrorCode);
void ConvertToText();
@@ -150,6 +152,8 @@ class CORE_EXPORT FileReaderLoader final : public ThreadableLoaderClient {
unsigned range_end_;

FileError::ErrorCode error_code_;
+
+ int64_t memory_usage_reported_to_v8_ = 0;
};

} // namespace blink

[dmu...@chromium.org]

WDYT?


https://codereview.chromium.org/2837873007/diff/20001/third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp
File third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp
(right):

https://codereview.chromium.org/2837873007/diff/20001/third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp#newcode352
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/

[mich...@chromium.org]

https://codereview.chromium.org/2837873007/diff/20001/third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp
File third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp
(right):

https://codereview.chromium.org/2837873007/diff/20001/third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp#newcode352
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/Source/core/fileapi/FileReaderLoader.h
File third_party/WebKit/Source/core/fileapi/FileReaderLoader.h (right):

https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Source/core/fileapi/FileReaderLoader.h#newcode150
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

https://codereview.chromium.org/2837873007/

[dmu...@chromium.org]

lgtm with nits


https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp
File third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp
(right):

https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp#newcode156
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/Source/core/fileapi/FileReaderLoader.cpp#newcode318
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/Source/core/fileapi/FileReaderLoader.cpp#newcode339
third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp:339:
SetStringResult(ConvertToText());
Do you need a std::move here?

https://codereview.chromium.org/2837873007/

[dmu...@chromium.org]

https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Source/core/fileapi/FileReaderLoader.h
File third_party/WebKit/Source/core/fileapi/FileReaderLoader.h (right):

https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Source/core/fileapi/FileReaderLoader.h#newcode150
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.

https://codereview.chromium.org/2837873007/

[mich...@chromium.org]

thnx

https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp
File third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp
(right):

https://codereview.chromium.org/2837873007/diff/40001/third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp#newcode156
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/Source/core/fileapi/FileReaderLoader.cpp#newcode318
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/Source/core/fileapi/FileReaderLoader.cpp#newcode339
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.

https://codereview.chromium.org/2837873007/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2837873007/60001

https://codereview.chromium.org/2837873007/

[dmu...@chromium.org]

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?

https://codereview.chromium.org/2837873007/

[commit-bot@chromium.org via codereview.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_presubmit/builds/423820)

https://codereview.chromium.org/2837873007/

[mich...@chromium.org]

like we talked about offline... there is no copy of the data being made here...
a ref is added str.impl_ is all

https://codereview.chromium.org/2837873007/

[mich...@chromium.org]

@kinuko, for an OWNERS review, thnx

https://codereview.chromium.org/2837873007/

[kin...@chromium.org]

lgtm

https://codereview.chromium.org/2837873007/

[commit-bot@chromium.org via codereview.chromium.org]

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/93af0c350008bf6e45ca995a8d88d161c8a384e0

https://codereview.chromium.org/2837873007/