Reserve Vector's capacity manually in CachedMetadata (issue 2258743002 by yhirano@chromium.org)

1 view
Skip to first unread message

yhi...@chromium.org

unread,
Aug 18, 2016, 2:10:48 AM8/18/16
to esp...@chromium.org, chromium...@chromium.org, mich...@chromium.org, jsbell+ser...@chromium.org, tyoshin...@chromium.org, tz...@chromium.org, servicewor...@chromium.org, kinuko+ser...@chromium.org, nhi...@chromium.org, fal...@chromium.org, har...@chromium.org, blink-revie...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, blink-...@chromium.org, loading-re...@chromium.org, jap...@chromium.org
Reviewers: esprehn
CL: https://codereview.chromium.org/2258743002/

Description:
Reserve Vector's capacity manually in CachedMetadata

As CachedMetaData::m_serializedData is set only at the initialization timing,
we can call Vector<char>::reserveCapacity to save memory consumption without
loosing performance.

This CL includes other cleanups:

- Renaming methods,
- Replacing error handling logic with DCHECKs,
- Replacing unsigned with uint32_t.

BUG=636462

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+88, -79 lines):
M third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.h
M third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp
M third_party/WebKit/Source/core/core.gypi
M third_party/WebKit/Source/core/fetch/CachedMetadata.h
A third_party/WebKit/Source/core/fetch/CachedMetadata.cpp
M third_party/WebKit/Source/core/fetch/CachedMetadataHandler.h
M third_party/WebKit/Source/core/fetch/Resource.cpp
M third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerScriptCachedMetadataHandler.h
M third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerScriptCachedMetadataHandler.cpp


har...@chromium.org

unread,
Aug 18, 2016, 2:15:19 AM8/18/16
to yhi...@chromium.org, esp...@chromium.org, chromium...@chromium.org, mich...@chromium.org, jsbell+ser...@chromium.org, tyoshin...@chromium.org, tz...@chromium.org, servicewor...@chromium.org, kinuko+ser...@chromium.org, nhi...@chromium.org, fal...@chromium.org, blink-revie...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, blink-...@chromium.org, loading-re...@chromium.org, jap...@chromium.org

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

unread,
Aug 25, 2016, 12:40:05 AM8/25/16
to yhi...@chromium.org, esp...@chromium.org, commi...@chromium.org, chromium...@chromium.org, mich...@chromium.org, jsbell+ser...@chromium.org, tyoshin...@chromium.org, tz...@chromium.org, servicewor...@chromium.org, kinuko+ser...@chromium.org, nhi...@chromium.org, fal...@chromium.org, blink-revie...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, blink-...@chromium.org, loading-re...@chromium.org, jap...@chromium.org

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

unread,
Aug 25, 2016, 12:47:14 AM8/25/16
to yhi...@chromium.org, esp...@chromium.org, commi...@chromium.org, chromium...@chromium.org, mich...@chromium.org, jsbell+ser...@chromium.org, tyoshin...@chromium.org, tz...@chromium.org, servicewor...@chromium.org, kinuko+ser...@chromium.org, nhi...@chromium.org, fal...@chromium.org, blink-revie...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, blink-...@chromium.org, loading-re...@chromium.org, jap...@chromium.org
Try jobs failed on following builders:
linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_compile_dbg_ng/builds/252547)

https://codereview.chromium.org/2258743002/

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

unread,
Aug 25, 2016, 12:49:26 AM8/25/16
to yhi...@chromium.org, esp...@chromium.org, commi...@chromium.org, chromium...@chromium.org, mich...@chromium.org, jsbell+ser...@chromium.org, tyoshin...@chromium.org, tz...@chromium.org, servicewor...@chromium.org, kinuko+ser...@chromium.org, nhi...@chromium.org, fal...@chromium.org, blink-revie...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, blink-...@chromium.org, loading-re...@chromium.org, jap...@chromium.org

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

unread,
Aug 25, 2016, 5:18:00 AM8/25/16
to yhi...@chromium.org, esp...@chromium.org, commi...@chromium.org, chromium...@chromium.org, mich...@chromium.org, jsbell+ser...@chromium.org, tyoshin...@chromium.org, tz...@chromium.org, servicewor...@chromium.org, kinuko+ser...@chromium.org, nhi...@chromium.org, fal...@chromium.org, blink-revie...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, blink-...@chromium.org, loading-re...@chromium.org, jap...@chromium.org
Try jobs failed on following builders:

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

unread,
Aug 25, 2016, 5:18:55 AM8/25/16
to yhi...@chromium.org, esp...@chromium.org, commi...@chromium.org, chromium...@chromium.org, mich...@chromium.org, jsbell+ser...@chromium.org, tyoshin...@chromium.org, tz...@chromium.org, servicewor...@chromium.org, kinuko+ser...@chromium.org, nhi...@chromium.org, fal...@chromium.org, blink-revie...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, blink-...@chromium.org, loading-re...@chromium.org, jap...@chromium.org

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

unread,
Aug 25, 2016, 9:08:12 AM8/25/16
to yhi...@chromium.org, esp...@chromium.org, commi...@chromium.org, chromium...@chromium.org, mich...@chromium.org, jsbell+ser...@chromium.org, tyoshin...@chromium.org, tz...@chromium.org, servicewor...@chromium.org, kinuko+ser...@chromium.org, nhi...@chromium.org, fal...@chromium.org, blink-revie...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, blink-...@chromium.org, loading-re...@chromium.org, jap...@chromium.org
Try jobs failed on following builders:
win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,

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

unread,
Aug 25, 2016, 9:12:28 AM8/25/16
to yhi...@chromium.org, esp...@chromium.org, commi...@chromium.org, chromium...@chromium.org, mich...@chromium.org, jsbell+ser...@chromium.org, tyoshin...@chromium.org, tz...@chromium.org, servicewor...@chromium.org, kinuko+ser...@chromium.org, nhi...@chromium.org, fal...@chromium.org, blink-revie...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, blink-...@chromium.org, loading-re...@chromium.org, jap...@chromium.org

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

unread,
Aug 25, 2016, 11:38:37 AM8/25/16
to yhi...@chromium.org, esp...@chromium.org, commi...@chromium.org, chromium...@chromium.org, mich...@chromium.org, jsbell+ser...@chromium.org, tyoshin...@chromium.org, tz...@chromium.org, servicewor...@chromium.org, kinuko+ser...@chromium.org, nhi...@chromium.org, fal...@chromium.org, blink-revie...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, blink-...@chromium.org, loading-re...@chromium.org, jap...@chromium.org
Failed to apply patch for
third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:
While running git apply --index -3 -p1;
error: patch failed:
third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:137
Falling back to three-way merge...
Applied patch to
'third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp' with conflicts.
U third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp

Patch: third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp
Index: third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp
b/third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp
index
f7258b0a58a5c024c2a58b189e65bfc0ef3c2686..0d76057d3df2b88a28fc45c4c9e08432e68aa35a
100644
--- a/third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp
@@ -137,7 +137,7 @@ v8::MaybeLocal<v8::Script>
compileWithoutOptions(V8CompileHistogram::Cacheabilit
}

// Compile a script, and consume a V8 cache that was generated previously.
-v8::MaybeLocal<v8::Script> compileAndConsumeCache(CachedMetadataHandler*
cacheHandler, unsigned tag, v8::ScriptCompiler::CompileOptions compileOptions,
v8::Isolate* isolate, v8::Local<v8::String> code, v8::ScriptOrigin origin)
+v8::MaybeLocal<v8::Script> compileAndConsumeCache(CachedMetadataHandler*
cacheHandler, uint32_t tag, v8::ScriptCompiler::CompileOptions compileOptions,
v8::Isolate* isolate, v8::Local<v8::String> code, v8::ScriptOrigin origin)
{
V8CompileHistogram histogramScope(V8CompileHistogram::Cacheable);
CachedMetadata* cachedMetadata = cacheHandler->cachedMetadata(tag);
@@ -153,7 +153,7 @@ v8::MaybeLocal<v8::Script>
compileAndConsumeCache(CachedMetadataHandler* cacheHa
}

// Compile a script, and produce a V8 cache for future use.
-v8::MaybeLocal<v8::Script> compileAndProduceCache(CachedMetadataHandler*
cacheHandler, unsigned tag, v8::ScriptCompiler::CompileOptions compileOptions,
CachedMetadataHandler::CacheType cacheType, v8::Isolate* isolate,
v8::Local<v8::String> code, v8::ScriptOrigin origin)
+v8::MaybeLocal<v8::Script> compileAndProduceCache(CachedMetadataHandler*
cacheHandler, uint32_t tag, v8::ScriptCompiler::CompileOptions compileOptions,
CachedMetadataHandler::CacheType cacheType, v8::Isolate* isolate,
v8::Local<v8::String> code, v8::ScriptOrigin origin)
{
V8CompileHistogram histogramScope(V8CompileHistogram::Cacheable);
v8::ScriptCompiler::Source source(code, origin);
@@ -176,7 +176,7 @@ v8::MaybeLocal<v8::Script>
compileAndProduceCache(CachedMetadataHandler* cacheHa

// Compile a script, and consume or produce a V8 Cache, depending on whether
the
// given resource already has cached data available.
-v8::MaybeLocal<v8::Script> compileAndConsumeOrProduce(CachedMetadataHandler*
cacheHandler, unsigned tag, v8::ScriptCompiler::CompileOptions consumeOptions,
v8::ScriptCompiler::CompileOptions produceOptions,
CachedMetadataHandler::CacheType cacheType, v8::Isolate* isolate,
v8::Local<v8::String> code, v8::ScriptOrigin origin)
+v8::MaybeLocal<v8::Script> compileAndConsumeOrProduce(CachedMetadataHandler*
cacheHandler, uint32_t tag, v8::ScriptCompiler::CompileOptions consumeOptions,
v8::ScriptCompiler::CompileOptions produceOptions,
CachedMetadataHandler::CacheType cacheType, v8::Isolate* isolate,
v8::Local<v8::String> code, v8::ScriptOrigin origin)
{
return cacheHandler->cachedMetadata(tag)
? compileAndConsumeCache(cacheHandler, tag, consumeOptions, isolate,
code, origin)
@@ -192,11 +192,11 @@ enum CacheTagKind {

static const int kCacheTagKindSize = 2;

-unsigned cacheTag(CacheTagKind kind, CachedMetadataHandler* cacheHandler)
+uint32_t cacheTag(CacheTagKind kind, CachedMetadataHandler* cacheHandler)
{
static_assert((1 << kCacheTagKindSize) >= CacheTagLast, "CacheTagLast must
be large enough");

- static unsigned v8CacheDataVersion =
v8::ScriptCompiler::CachedDataVersionTag() << kCacheTagKindSize;
+ static uint32_t v8CacheDataVersion =
v8::ScriptCompiler::CachedDataVersionTag() << kCacheTagKindSize;

// A script can be (successfully) interpreted with different encodings,
// depending on the page it appears in. The cache doesn't know anything
@@ -210,7 +210,7 @@ unsigned cacheTag(CacheTagKind kind, CachedMetadataHandler*
cacheHandler)
bool isResourceHotForCaching(CachedMetadataHandler* cacheHandler, int hotHours)
{
const double cacheWithinSeconds = hotHours * 60 * 60;
- unsigned tag = cacheTag(CacheTagTimeStamp, cacheHandler);
+ uint32_t tag = cacheTag(CacheTagTimeStamp, cacheHandler);
CachedMetadata* cachedMetadata = cacheHandler->cachedMetadata(tag);
if (!cachedMetadata)
return false;
@@ -304,7 +304,7 @@ std::unique_ptr<CompileFn>
selectCompileFunction(V8CacheOptions cacheOptions, Ca
case V8CacheOptionsAlways: {
// Use code caching for recently seen resources.
// Use compression depending on the cache option.
- unsigned codeCacheTag = cacheTag(CacheTagCode, cacheHandler);
+ uint32_t codeCacheTag = cacheTag(CacheTagCode, cacheHandler);
CachedMetadata* codeCache = cacheHandler->cachedMetadata(codeCacheTag);
if (codeCache)
return bind(compileAndConsumeCache, wrapPersistent(cacheHandler),
codeCacheTag, v8::ScriptCompiler::kConsumeCodeCache);
@@ -563,12 +563,12 @@ v8::MaybeLocal<v8::Object>
V8ScriptRunner::instantiateObjectInDocument(v8::Isola
return result;
}

-unsigned V8ScriptRunner::tagForParserCache(CachedMetadataHandler* cacheHandler)
+uint32_t V8ScriptRunner::tagForParserCache(CachedMetadataHandler* cacheHandler)
{
return cacheTag(CacheTagParser, cacheHandler);
}

-unsigned V8ScriptRunner::tagForCodeCache(CachedMetadataHandler* cacheHandler)
+uint32_t V8ScriptRunner::tagForCodeCache(CachedMetadataHandler* cacheHandler)
{
return cacheTag(CacheTagCode, cacheHandler);
}
@@ -577,7 +577,7 @@ unsigned
V8ScriptRunner::tagForCodeCache(CachedMetadataHandler* cacheHandler)
void V8ScriptRunner::setCacheTimeStamp(CachedMetadataHandler* cacheHandler)
{
double now = WTF::currentTime();
- unsigned tag = cacheTag(CacheTagTimeStamp, cacheHandler);
+ uint32_t tag = cacheTag(CacheTagTimeStamp, cacheHandler);
cacheHandler->clearCachedMetadata(CachedMetadataHandler::CacheLocally);
cacheHandler->setCachedMetadata(tag, reinterpret_cast<char*>(&now),
sizeof(now), CachedMetadataHandler::SendToPlatform);
}


https://codereview.chromium.org/2258743002/

esp...@chromium.org

unread,
Aug 26, 2016, 12:51:01 AM8/26/16
to yhi...@chromium.org, har...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, chromium...@chromium.org, fal...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, jap...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, loading-re...@chromium.org, mich...@chromium.org, nhi...@chromium.org, servicewor...@chromium.org, tyoshin...@chromium.org, tz...@chromium.org

https://codereview.chromium.org/2258743002/diff/80001/third_party/WebKit/Source/core/fetch/CachedMetadata.cpp
File third_party/WebKit/Source/core/fetch/CachedMetadata.cpp (right):

https://codereview.chromium.org/2258743002/diff/80001/third_party/WebKit/Source/core/fetch/CachedMetadata.cpp#newcode14
third_party/WebKit/Source/core/fetch/CachedMetadata.cpp:14:
DCHECK_GT(size, kDataStart);
DCHECK(data)

https://codereview.chromium.org/2258743002/diff/80001/third_party/WebKit/Source/core/fetch/CachedMetadata.cpp#newcode15
third_party/WebKit/Source/core/fetch/CachedMetadata.cpp:15:
m_serializedData.reserveCapacity(size);
reserveInitialCapacity

https://codereview.chromium.org/2258743002/diff/80001/third_party/WebKit/Source/core/fetch/CachedMetadata.cpp#newcode25
third_party/WebKit/Source/core/fetch/CachedMetadata.cpp:25:
m_serializedData.reserveCapacity(sizeof(uint32_t) + size);
reserveInitialCapacity.

https://codereview.chromium.org/2258743002/

yhi...@chromium.org

unread,
Aug 26, 2016, 4:50:54 AM8/26/16
to esp...@chromium.org, har...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, chromium...@chromium.org, fal...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, jap...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, loading-re...@chromium.org, mich...@chromium.org, nhi...@chromium.org, servicewor...@chromium.org, tyoshin...@chromium.org, tz...@chromium.org

https://codereview.chromium.org/2258743002/diff/80001/third_party/WebKit/Source/core/fetch/CachedMetadata.cpp
File third_party/WebKit/Source/core/fetch/CachedMetadata.cpp (right):

https://codereview.chromium.org/2258743002/diff/80001/third_party/WebKit/Source/core/fetch/CachedMetadata.cpp#newcode14
third_party/WebKit/Source/core/fetch/CachedMetadata.cpp:14:
DCHECK_GT(size, kDataStart);
On 2016/08/26 04:51:01, esprehn wrote:
> DCHECK(data)

Done.


https://codereview.chromium.org/2258743002/diff/80001/third_party/WebKit/Source/core/fetch/CachedMetadata.cpp#newcode15
third_party/WebKit/Source/core/fetch/CachedMetadata.cpp:15:
m_serializedData.reserveCapacity(size);
On 2016/08/26 04:51:01, esprehn wrote:
> reserveInitialCapacity

Done.


https://codereview.chromium.org/2258743002/diff/80001/third_party/WebKit/Source/core/fetch/CachedMetadata.cpp#newcode25
third_party/WebKit/Source/core/fetch/CachedMetadata.cpp:25:
m_serializedData.reserveCapacity(sizeof(uint32_t) + size);
On 2016/08/26 04:51:01, esprehn wrote:
> reserveInitialCapacity.

Done.

https://codereview.chromium.org/2258743002/

esp...@chromium.org

unread,
Aug 30, 2016, 5:26:01 PM8/30/16
to yhi...@chromium.org, har...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, chromium...@chromium.org, fal...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, jap...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, loading-re...@chromium.org, mich...@chromium.org, nhi...@chromium.org, servicewor...@chromium.org, tyoshin...@chromium.org, tz...@chromium.org

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

unread,
Aug 30, 2016, 8:43:40 PM8/30/16
to yhi...@chromium.org, esp...@chromium.org, har...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, chromium...@chromium.org, fal...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, jap...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, loading-re...@chromium.org, mich...@chromium.org, nhi...@chromium.org, servicewor...@chromium.org, tyoshin...@chromium.org, tz...@chromium.org
CQ is trying da patch. Follow status at

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

unread,
Aug 30, 2016, 10:41:48 PM8/30/16
to yhi...@chromium.org, esp...@chromium.org, har...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, chromium...@chromium.org, fal...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, jap...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, loading-re...@chromium.org, mich...@chromium.org, nhi...@chromium.org, servicewor...@chromium.org, tyoshin...@chromium.org, tz...@chromium.org
Committed patchset #5 (id:100001)

https://codereview.chromium.org/2258743002/

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

unread,
Aug 30, 2016, 10:43:05 PM8/30/16
to yhi...@chromium.org, esp...@chromium.org, har...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, chromium...@chromium.org, fal...@chromium.org, gavinp...@chromium.org, horo+...@chromium.org, jap...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, loading-re...@chromium.org, mich...@chromium.org, nhi...@chromium.org, servicewor...@chromium.org, tyoshin...@chromium.org, tz...@chromium.org
Patchset 5 (id:??) landed as
https://crrev.com/2805c285bf5a0b681c2ad648c0d5506aa26985d1
Cr-Commit-Position: refs/heads/master@{#415544}

https://codereview.chromium.org/2258743002/
Reply all
Reply to author
Forward
0 new messages