Bug 163245 – [DOMJIT][JSC] Explore the way to embed nodeType into JSC::JSType in WebCore

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

Node.prototype.nodeType() is frequently called in Dromaeo don-query.html, but it is implemented in virtual function call.
I think we could implement it very efficiently... like, embedding this type into JSC::JSType.
In WebCore, we extend JSC::JSType in JSDOMWrapper.h. Like adding JSElementType.

Note that nodeType is frequently used in reality. Usually it is used to check whether the given object is DOM node.
For example, jQuery's $ function is like this.

		// HANDLE: $(""), $(null), $(undefined), $(false)
		if ( !selector ) {
                    ...
		}

		// Method init() accepts an alternate rootjQuery
		// so migrate can support jQuery.sub (gh-2101)
		root = root || rootjQuery;

		// Handle HTML strings
		if ( typeof selector === "string" ) {
                    ...
		// HANDLE: $(DOMElement)
		} else if ( selector.nodeType ) {                  <--------- THIS
                    ...
		// HANDLE: $(function)
		// Shortcut for document ready
		} else if ( jQuery.isFunction( selector ) ) {
                    ...
		}
                ...

[Yusuke Suzuki]

The cleanest way is just putting nodeType to Node. But Node's flag it already exhausted. And we should not add a new one since it significantly enlarges the memory consumption.

[Yusuke Suzuki]

WebCore's extra JSType is very similar to NodeType. Like, JSElementType, JSNodeType, JSDocumentWrapperType etc.
So I think we have a chance to represent NodeType with JSType.

[Yusuke Suzuki]

Early evaluation with Dromaeo dom-query.html

Patched:

getElementById:Runs -> [6043, 6139, 6198, 6202, 6207] runs/s
    mean: 6157.8 runs/s
    median: 6198 runs/s
    stdev: 69.86200684205961 runs/s
    min: 6043 runs/s
    max: 6207 runs/s

getElementById (not in document):Runs -> [6626, 6664, 6680, 6688, 6717] runs/s
    mean: 6675 runs/s
    median: 6680 runs/s
    stdev: 33.46640106136295 runs/s
    min: 6626 runs/s
    max: 6717 runs/s

getElementsByTagName(div):Runs -> [431829, 435339, 440814, 455979, 456323] runs/s
    mean: 444056.8 runs/s
    median: 440814 runs/s
    stdev: 11496.014491988084 runs/s
    min: 431829 runs/s
    max: 456323 runs/s

getElementsByTagName(p):Runs -> [438485, 439710, 444404, 447107, 449263] runs/s
    mean: 443793.8 runs/s
    median: 444404 runs/s
    stdev: 4640.112250797395 runs/s
    min: 438485 runs/s
    max: 449263 runs/s

getElementsByTagName(a):Runs -> [445398, 446686, 448782, 449314, 456469] runs/s
    mean: 449329.8 runs/s
    median: 448782 runs/s
    stdev: 4292.937246221991 runs/s
    min: 445398 runs/s
    max: 456469 runs/s

getElementsByTagName(*):Runs -> [421789, 423986, 427341, 430128, 431760] runs/s
    mean: 427000.8 runs/s
    median: 427341 runs/s
    stdev: 4147.197210164959 runs/s
    min: 421789 runs/s
    max: 431760 runs/s

getElementsByTagName (not in document):Runs -> [731858, 772983, 776813, 781652, 787752] runs/s
    mean: 770211.6 runs/s
    median: 776813 runs/s
    stdev: 22140.4954619358 runs/s
    min: 731858 runs/s
    max: 787752 runs/s

getElementsByName:Runs -> [5860, 6317, 6356, 6368, 6394] runs/s
    mean: 6259 runs/s
    median: 6356 runs/s
    stdev: 224.7665455533807 runs/s
    min: 5860 runs/s
    max: 6394 runs/s

getElementsByName (not in document):Runs -> [7388, 7519, 7658, 8196, 8223] runs/s
    mean: 7796.8 runs/s
    median: 7658 runs/s
    stdev: 388.7681828545128 runs/s
    min: 7388 runs/s
    max: 8223 runs/s


:Runs -> [1579.8208506001133, 1626.7710342620671, 1641.1431400710333, 1666.485339816947, 1671.7244248328077] runs/s
    mean: 1637.1889579165938 runs/s
    median: 1641.1431400710333 runs/s
    stdev: 36.96702887178485 runs/s
    min: 1579.8208506001133 runs/s
    max: 1671.7244248328077 runs/s


Baseline:

getElementById:Runs -> [4996, 5097, 5107, 5154, 5198] runs/s
    mean: 5110.4 runs/s
    median: 5107 runs/s
    stdev: 75.58637443349166 runs/s
    min: 4996 runs/s
    max: 5198 runs/s

getElementById (not in document):Runs -> [6513, 6551, 6609, 6616, 6958] runs/s
    mean: 6649.4 runs/s
    median: 6609 runs/s
    stdev: 177.67188860368435 runs/s
    min: 6513 runs/s
    max: 6958 runs/s

getElementsByTagName(div):Runs -> [371366, 373887, 374717, 384777, 390212] runs/s
    mean: 378991.8 runs/s
    median: 374717 runs/s
    stdev: 8090.845919927044 runs/s
    min: 371366 runs/s
    max: 390212 runs/s

getElementsByTagName(p):Runs -> [375126, 383549, 400543, 404636, 404973] runs/s
    mean: 393765.4 runs/s
    median: 400543 runs/s
    stdev: 13615.4155757362 runs/s
    min: 375126 runs/s
    max: 404973 runs/s

getElementsByTagName(a):Runs -> [369938, 407215, 409685, 410603, 416075] runs/s
    mean: 402703.2 runs/s
    median: 409685 runs/s
    stdev: 18600.215568643278 runs/s
    min: 369938 runs/s
    max: 416075 runs/s

getElementsByTagName(*):Runs -> [385668, 387601, 392433, 398342, 403250] runs/s
    mean: 393458.8 runs/s
    median: 392433 runs/s
    stdev: 7344.656064105391 runs/s
    min: 385668 runs/s
    max: 403250 runs/s

getElementsByTagName (not in document):Runs -> [718961, 737701, 759983, 764276, 777297] runs/s
    mean: 751643.6 runs/s
    median: 759983 runs/s
    stdev: 23182.797993339816 runs/s
    min: 718961 runs/s
    max: 777297 runs/s

getElementsByName:Runs -> [5265, 5292, 5296, 5327, 5333] runs/s
    mean: 5302.6 runs/s
    median: 5296 runs/s
    stdev: 27.79028607265496 runs/s
    min: 5265 runs/s
    max: 5333 runs/s

getElementsByName (not in document):Runs -> [7880, 7889, 7928, 7975, 7999] runs/s
    mean: 7934.2 runs/s
    median: 7928 runs/s
    stdev: 52.16032975355891 runs/s
    min: 7880 runs/s
    max: 7999 runs/s


:Runs -> [1465.0259452526852, 1478.767959301689, 1484.6802981963529, 1493.4019246547011, 1515.541250761825] runs/s
    mean: 1487.4834756334508 runs/s
    median: 1484.6802981963529 runs/s
    stdev: 18.777815923634538 runs/s
    min: 1465.0259452526852 runs/s
    max: 1515.541250761825 runs/s

[Saam Barati]

nice!

[Yusuke Suzuki]

Created attachment 291340 [details]
Patch

WIP

[WebKit Commit Bot]

Attachment 291340 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeConstants.h:32:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/NodeConstants.h:33:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/NodeConstants.h:34:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/NodeConstants.h:35:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/NodeConstants.h:36:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/NodeConstants.h:37:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/NodeConstants.h:38:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/NodeConstants.h:39:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/NodeConstants.h:40:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/NodeConstants.h:44:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/NodeConstants.h:45:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/NodeConstants.h:46:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 12 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 291381 [details]
Patch

[WebKit Commit Bot]

Attachment 291381 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeConstants.h:32:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/NodeConstants.h:33:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/NodeConstants.h:34:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/NodeConstants.h:35:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/NodeConstants.h:36:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/NodeConstants.h:37:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/NodeConstants.h:38:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/NodeConstants.h:39:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/NodeConstants.h:40:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/NodeConstants.h:44:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/NodeConstants.h:45:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/NodeConstants.h:46:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 12 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 291381 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291381&action=review

This is awesome!

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7152
> +    // FIXME: patchpoint should have a way to tell this can reuse "base" register.
> +    // Teaching DFG about DOMJIT::Patchpoint clobber information is nice.

Note that those register reuse optimizations are really only useful if you have <=4 instructions in your generator.  I think you have more than that.  So, if there is an extra register-register move or an extra spill that could have been avoided by reuse, it probably won't matter that much.  On the other hand, generating correct code when registers get reused is super hard.  We often get bugs in such code in the DFG, and we usually fix them by removing the reuse.  To my knowledge, those reuse removal changes almost never cause perf regressions.  They only cause perf regressions for simple things like add/sub, hence my rule of <=4 instructions.

[Yusuke Suzuki]

Comment on attachment 291381 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291381&action=review

Thanks!

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7152
>> +    // Teaching DFG about DOMJIT::Patchpoint clobber information is nice.
> 
> Note that those register reuse optimizations are really only useful if you have <=4 instructions in your generator.  I think you have more than that.  So, if there is an extra register-register move or an extra spill that could have been avoided by reuse, it probably won't matter that much.  On the other hand, generating correct code when registers get reused is super hard.  We often get bugs in such code in the DFG, and we usually fix them by removing the reuse.  To my knowledge, those reuse removal changes almost never cause perf regressions.  They only cause perf regressions for simple things like add/sub, hence my rule of <=4 instructions.

Oh! So, in many CallDOM operations, reusing registers do not matter, nice.
And yup, reusing easily causes a bug... I'll drop this FIXME.

[Yusuke Suzuki]

Committed r207239: <http://trac.webkit.org/changeset/207239>

[Yusuke Suzuki]

Oops, I missed to git-add expect.txt file.
I'll add it soon.

[Yusuke Suzuki]

Committed r207261: <http://trac.webkit.org/changeset/207261>

[Michael Catanzaro]

Hi, please check if this might have caused bug #163826, thanks!