Created attachment 133799 [details] Performance test createTextNode(), createElement(), cloneNode() etc, which creates a new DOM object every time, are much slower in V8 than in JSC. There are the main methods used in Dromaeo/dom-modify.html. The results of the attached performance test are as follows: Chromium/V8/Mac: createTextNode : median=317ms (mean=439.08ms, min=303ms, max=3126ms) createElement : median=403ms (mean=695.70ms, min=398ms, max=5615ms) cloneNode : median=384ms (mean=577.96ms, min=372ms, max=5313ms) Safari/JSC/Mac: createTextNode : median=142ms (mean=141.04ms, min=110ms, max=168ms) createElement : median=234ms (mean=245.74ms, min=219ms, max=305ms) cloneNode : median=210ms (mean=213.36ms, min=204ms, max=284ms) Chromium results are really noisy due to the unsteadiness of V8's GC. Let us focus on the median of the measured times. I'll discuss the GC issues with the V8 team later.
Created attachment 133800 [details] Patch
Created attachment 133802 [details] Patch
Comment on attachment 133802 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133802&action=review Please don't forget to run run-binding-tests > Source/WebCore/bindings/v8/V8Proxy.cpp:648 > + windowShell()->initContextIfNeeded(); > + return windowShell()->context(); I know we currently only get here from the code generated by the code generator which tests for IsNodeSubType so we cannot get here from a worker. But I'm concerned about us special casing so many things. For now I would be OK with an assert. In the long term I think it is bad that we tie the context to the windowShell like this.
Comment on attachment 133802 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133802&action=review > Source/WebCore/bindings/v8/V8Proxy.cpp:642 > + RefPtr<SharedPersistent<v8::Context> > context = isolatedContext->sharedContext(); Why is this a RefPtr? > Source/WebCore/bindings/v8/V8Proxy.h:248 > + // context() and persistentContext() return the same context. > + // While context() returns a newly created Local handle, > + // persistentContext() returns an existing Persistent handle. > v8::Local<v8::Context> context(); > + v8::Handle<v8::Context> persistentContext(); I'm pretty sure we use a new local handle here because we're worried about our persistent handle getting disposed while we've entered it. Consider what happens if we run page JavaScript while having entered our persistent handle and that JavaScript causes the Frame and everything to be torn down. We'll dispose the persistent handle, which could cause the context to be destroyed. I'm pretty sure we've had crashes like that before and that's why we use a new local handle (to ensure that the context stays around).
Created attachment 133941 [details] Patch
Created attachment 133943 [details] Patch
(In reply to comment #4) > > Source/WebCore/bindings/v8/V8Proxy.cpp:642 > > + RefPtr<SharedPersistent<v8::Context> > context = isolatedContext->sharedContext(); > > Why is this a RefPtr? Fixed. > > Source/WebCore/bindings/v8/V8Proxy.h:248 > > + // context() and persistentContext() return the same context. > > + // While context() returns a newly created Local handle, > > + // persistentContext() returns an existing Persistent handle. > > v8::Local<v8::Context> context(); > > + v8::Handle<v8::Context> persistentContext(); > > I'm pretty sure we use a new local handle here because we're worried about our persistent handle getting disposed while we've entered it. Consider what happens if we run page JavaScript while having entered our persistent handle and that JavaScript causes the Frame and everything to be torn down. We'll dispose the persistent handle, which could cause the context to be destroyed. Fixed. Just fixed a code in a cold path, and thus no performance change. (In reply to comment #3) > Please don't forget to run run-binding-tests Updated. > > Source/WebCore/bindings/v8/V8Proxy.cpp:648 > > + windowShell()->initContextIfNeeded(); > > + return windowShell()->context(); > > I know we currently only get here from the code generated by the code generator which tests for IsNodeSubType so we cannot get here from a worker. For now I would be OK with an assert. How should we write the assert, to check if we are not in a worker?
> How should we write the assert, to check if we are not in a worker? ASSERT(isMainThread()); ?
Comment on attachment 133943 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133943&action=review > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3160 > + // For performance, we do not create a Local handle of the context > + // until we really need to enter the context. > + if (proxy) > + context = proxy->context(); Is there actually any way of getting here if proxy is false? It seems like context.IsEmpty() will be true in that case and we'll never reach this line. Another idea: Maybe we should move this code into V8Proxy? v8::Handle<v8::Context> context; if (proxy && !proxy->matchesCurrentContext()) { context = proxy->context(); if (context) context->Enter(); } where V8Proxy::matchesCurrentContext is something like: v8::Handle<v8::Context> context = windowShell()->context(); return !context->IsEmpty() && context == context->GetCurrent(); (Although likely more complicated to deal with isolated worlds.) That would save all the trickiness around having two accessors on V8Proxy for the context.
Created attachment 133972 [details] Patch
(In reply to comment #9) > v8::Handle<v8::Context> context; > if (proxy && !proxy->matchesCurrentContext()) { > context = proxy->context(); > if (context) > context->Enter(); > } > > where V8Proxy::matchesCurrentContext is something like: > > v8::Handle<v8::Context> context = windowShell()->context(); > return !context->IsEmpty() && context == context->GetCurrent(); > > That would save all the trickiness around having two accessors on V8Proxy for the context. Done.
Comment on attachment 133972 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133972&action=review > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3150 > + bool contextEntered = false; Can't we just use context.IsEmpty() instead of introducing this bool?
Created attachment 133986 [details] patch for landing
Comment on attachment 133986 [details] patch for landing Clearing flags on attachment: 133986 Committed r112218: <http://trac.webkit.org/changeset/112218>