106667 2013-01-11 08:29:23 -0800 [V8] Slightly optimize getWrapperFast() 2013-01-17 09:58:20 -0800 1 1 1 Unclassified WebKit WebCore JavaScript 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 haraken haraken abarth dcarney japhet webkit.review.bot oldest_to_newest 805279 0 haraken 2013-01-11 08:29:23 -0800 Slightly optimize getWrapperFast(). 805283 1 182348 haraken 2013-01-11 08:41:35 -0800 Created attachment 182348 Patch 805373 2 182348 abarth 2013-01-11 10:26:39 -0800 Comment on attachment 182348 Patch ok 805384 3 haraken 2013-01-11 10:34:45 -0800 BTW, what do you think about the following idea? - Use a class id to indicate whether we're in the main world or not. Specifically, we can use v8DOMNodeInMainWorldClassId, v8DOMObjectInMainWorldClassId, v8DOMNodeInIsolateWorldClassId, v8DOMObjectInIsolateWorldClassId. Then we can know whether we're in the main world or not by looking at a class id of info.Holder(). This will simplify a bunch of methods in DOMDataStore.h. I don't think this will improve performance because the number of if branches doesn't change, but it will simplify the confusing if conditions in DOMDataStore.h. 805391 4 182348 webkit.review.bot 2013-01-11 10:38:48 -0800 Comment on attachment 182348 Patch Clearing flags on attachment: 182348 Committed r139458: <http://trac.webkit.org/changeset/139458> 805392 5 webkit.review.bot 2013-01-11 10:38:52 -0800 All reviewed patches have been landed. Closing bug. 805412 6 abarth 2013-01-11 10:52:37 -0800 > BTW, what do you think about the following idea? I'd rather not pack more bits into the class ID unless there's a performance gain. The class ID is a very limited space, and we want to save it for things we really need. Another idea is to use different ObjectTemplates for the main world and for isolated worlds. That way we could have two implementations of nextSiblingCallback, one for the main world and one for isolated worlds. The one for the main world could then skip the "am I in the main world" branch entirely and grab at the inline wrapper directly. 805423 7 haraken 2013-01-11 11:02:16 -0800 (In reply to comment #6) > > BTW, what do you think about the following idea? > > I'd rather not pack more bits into the class ID unless there's a performance gain. The class ID is a very limited space, and we want to save it for things we really need. This isn't quite right. A class ID has 16 bit (although I am trying to reduce the size to 8 bit just right now). > Another idea is to use different ObjectTemplates for the main world and for isolated worlds. That way we could have two implementations of nextSiblingCallback, one for the main world and one for isolated worlds. The one for the main world could then skip the "am I in the main world" branch entirely and grab at the inline wrapper directly. Sounds like a better approach. (This is the approach Dan is also proposing.) My only concern is that it will complicate the generated code. Specifically, we will need to generate something like the following code for all getters/setters/methods, affecting custom bindings: template<WorldType world> struct firstChildAttrGetterStruct { static v8::Handle<v8::Value> firstChildAttrGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info) { Node* imp = V8Node::toNative(info.Holder()); return toV8Fast<world>(imp->firstChild(), info, imp); } }; template<> struct firstChildAttrGetterStruct<MainWorld>; template<> struct firstChildAttrGetterStruct<IsolateWorld>; 805472 8 abarth 2013-01-11 11:42:55 -0800 We can also just generate two copies of the function and skip the templates. 809305 9 haraken 2013-01-17 06:32:27 -0800 (In reply to comment #8) > We can also just generate two copies of the function and skip the templates. For experiment, I removed the following lines to observe the performance impact of generating getters/setters for the main world: static v8::Handle<v8::Object> getWrapperFast(T* object, const HolderContainer& container, Wrappable* holder) { /* if ((!DOMWrapperWorld::isolatedWorldsExist() && isMainWorldObject(object)) || holderContainsWrapper(container, holder)) { */ if (mainWorldWrapperIsStoredInObject(object)) return getWrapperFromObject(object); return mainWorldStore()->m_wrapperMap.get(object); /* } return current(container.GetIsolate())->get(object); */ } As a result, a micro benchmark for .firstChild improved from 14.1 ns to 13.7 ns. On the other hand, I couldn't observe any performance difference in Dromaeo/dom-traverse. So it might not be worth doing the optimization. 809413 10 abarth 2013-01-17 09:58:20 -0800 Interesting. Thanks! 182348 2013-01-11 08:41:35 -0800 2013-01-11 10:38:48 -0800 Patch bug-106667-20130111173837.patch text/plain 2719 haraken U3VidmVyc2lvbiBSZXZpc2lvbjogMTM5MTgzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNTc3MWY2YTNkZDZjNTY2 ZjBkZDMxZTI1OWRmNmI4YmYyMWNjNjA4My4uNzRjNjIxZTIwZjcxMGEyYmI5MmI4ZDE5ZTczNDcy OWY1ZWFlNDNjMyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIyIEBACisyMDEzLTAxLTExICBLZW50 YXJvIEhhcmEgIDxoYXJha2VuQGNocm9taXVtLm9yZz4KKworICAgICAgICBbVjhdIFNsaWdodGx5 IG9wdGltaXplIGdldFdyYXBwZXJGYXN0KCkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTEwNjY2NworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgIFRoaXMgcGF0Y2ggaW1wcm92ZXMgYW4gaWYgY29uZGl0aW9uIGlu IGdldFdyYXBwZXJGYXN0KCksCisgICAgICAgIGFzIGNvbW1lbnRlZCBpbiBET01EYXRhU3RvcmUu aC4KKworICAgICAgICBUaGlzIHBhdGNoIGltcHJvdmVzIHBlcmZvcm1hbmNlIG9mIGRpdi5maXJz dENoaWxkIGZyb20KKyAgICAgICAgMTUuMSBucyB0byAxNC4wIG5zICgrNy44JSksIGFsdGhvdWdo IEkgY291bGRuJ3Qgb2JzZXJ2ZQorICAgICAgICBwZXJmb3JtYW5jZSBpbXByb3ZlbWVudCBpbiBE cm9tYWVvL2RvbS10cmF2ZXJzZS4KKworICAgICAgICBObyB0ZXN0cy4gTm8gY2hhbmdlIGluIGJl aGF2aW9yLgorCisgICAgICAgICogYmluZGluZ3MvdjgvRE9NRGF0YVN0b3JlLmg6CisgICAgICAg IChXZWJDb3JlOjpET01EYXRhU3RvcmU6OmdldFdyYXBwZXJGYXN0KToKKwogMjAxMy0wMS0wOSAg WmVubyBBbGJpc3NlciAgPHplbm9Ad2Via2l0Lm9yZz4KIAogICAgICAgICBbUXRdW01hY10gR3Jh cGhpY3NTdXJmYWNlIGRvZXMgbm90IG5lZWQgZ2xFbmFibGUvZ2xEaXNhYmxlIGZvciB0ZXh0dXJl IHRhcmdldHMuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy92OC9ET01EYXRh U3RvcmUuaCBiL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL3Y4L0RPTURhdGFTdG9yZS5oCmluZGV4 IDVkZDYyNWJjNTg2YTQyNDdiNjg5YzIwNzMzMjVjY2E4NDEyNDJjMTIuLjQxNmU0NTA2YmY4MmJm ZGQ1YWM0NGU0ZTI5MjllYjFkZTc0YzQ2YmUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2Jp bmRpbmdzL3Y4L0RPTURhdGFTdG9yZS5oCisrKyBiL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL3Y4 L0RPTURhdGFTdG9yZS5oCkBAIC02NCwxNSArNjQsMTggQEAgcHVibGljOgogICAgIHRlbXBsYXRl PHR5cGVuYW1lIFQsIHR5cGVuYW1lIEhvbGRlckNvbnRhaW5lciwgdHlwZW5hbWUgV3JhcHBhYmxl PgogICAgIHN0YXRpYyB2ODo6SGFuZGxlPHY4OjpPYmplY3Q+IGdldFdyYXBwZXJGYXN0KFQqIG9i amVjdCwgY29uc3QgSG9sZGVyQ29udGFpbmVyJiBjb250YWluZXIsIFdyYXBwYWJsZSogaG9sZGVy KQogICAgIHsKLSAgICAgICAgLy8gV2hhdCB3ZSdkIHJlYWxseSBsaWtlIHRvIGNoZWNrIGhlcmUg aXMgd2hldGhlciB3ZSdyZSBpbiB0aGUgbWFpbiB3b3JsZCBvcgotICAgICAgICAvLyBpbiBhbiBp c29sYXRlZCB3b3JsZC4gVGhlIGZhc3Rlc3Qgd2F5IHdlIGtub3cgaG93IHRvIGRvIHRoYXQgaXMg dG8gY2hlY2sKLSAgICAgICAgLy8gd2hldGhlciB0aGUgd3JhcHBhYmxlJ3Mgd3JhcHBlciBpcyB0 aGUgc2FtZSBhcyB0aGUgaG9sZGVyCi0gICAgICAgIGlmIChob2xkZXJDb250YWluc1dyYXBwZXIo Y29udGFpbmVyLCBob2xkZXIpKSB7CisgICAgICAgIC8vIFdoYXQgd2UnZCByZWFsbHkgbGlrZSB0 byBjaGVjayBoZXJlIGlzIHdoZXRoZXIgd2UncmUgaW4gdGhlCisgICAgICAgIC8vIG1haW4gd29y bGQgb3IgaW4gYW4gaXNvbGF0ZWQgd29ybGQuIFRoZSBmYXN0ZXN0IHdheSB0byBkbyB0aGF0Cisg ICAgICAgIC8vIGlzIHRvIGNoZWNrIHRoYXQgdGhlcmUgaXMgbm8gaXNvbGF0ZWQgd29ybGQgYW5k IHRoZSAnb2JqZWN0JworICAgICAgICAvLyBpcyBhbiBvYmplY3QgdGhhdCBjYW4gZXhpc3QgaW4g dGhlIG1haW4gd29ybGQuIFRoZSBzZWNvbmQgZmFzdGVzdAorICAgICAgICAvLyB3YXkgaXMgdG8g Y2hlY2sgd2hldGhlciB0aGUgd3JhcHBhYmxlJ3Mgd3JhcHBlciBpcyB0aGUgc2FtZSBhcworICAg ICAgICAvLyB0aGUgaG9sZGVyLgorICAgICAgICBpZiAoKCFET01XcmFwcGVyV29ybGQ6Omlzb2xh dGVkV29ybGRzRXhpc3QoKSAmJiBpc01haW5Xb3JsZE9iamVjdChvYmplY3QpKSB8fCBob2xkZXJD b250YWluc1dyYXBwZXIoY29udGFpbmVyLCBob2xkZXIpKSB7CiAgICAgICAgICAgICBpZiAobWFp bldvcmxkV3JhcHBlcklzU3RvcmVkSW5PYmplY3Qob2JqZWN0KSkKICAgICAgICAgICAgICAgICBy ZXR1cm4gZ2V0V3JhcHBlckZyb21PYmplY3Qob2JqZWN0KTsKICAgICAgICAgICAgIHJldHVybiBt YWluV29ybGRTdG9yZSgpLT5tX3dyYXBwZXJNYXAuZ2V0KG9iamVjdCk7CiAgICAgICAgIH0KLSAg ICAgICAgcmV0dXJuIGdldFdyYXBwZXIob2JqZWN0LCBjb250YWluZXIuR2V0SXNvbGF0ZSgpKTsK KyAgICAgICAgcmV0dXJuIGN1cnJlbnQoY29udGFpbmVyLkdldElzb2xhdGUoKSktPmdldChvYmpl Y3QpOwogICAgIH0KIAogICAgIHRlbXBsYXRlPHR5cGVuYW1lIFQ+Cg==