157209 – ThisTDZMode is no longer needed

Bug 157209 - ThisTDZMode is no longer needed

Summary: ThisTDZMode is no longer needed

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	JavaScriptCore (show other bugs)
Version:	WebKit Nightly Build
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Yusuke Suzuki

URL:
Keywords:

Depends on:
Blocks:

Reported:	2016-04-29 15:45 PDT by Saam Barati
Modified:	2016-05-24 05:03 PDT (History)
CC List:	10 users (show)

See Also:

Attachments
patch (6.07 KB, patch) 2016-04-29 15:49 PDT, Saam Barati	saam: review-	Details \| Formatted Diff \| Diff
patch (30.98 KB, patch) 2016-04-29 17:07 PDT, Saam Barati	no flags	Details \| Formatted Diff \| Diff
Patch (44.41 KB, patch) 2016-05-22 07:30 PDT, Yusuke Suzuki	saam: review+	Details \| Formatted Diff \| Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Saam Barati 2016-04-29 15:45:40 PDT

...

Comment 1 Saam Barati 2016-04-29 15:49:59 PDT

Created attachment 277743 [details]
patch

Comment 2 Mark Lam 2016-04-29 16:01:56 PDT

Comment on attachment 277743 [details]
patch

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

r=me

> Source/JavaScriptCore/ChangeLog:11
> +        The field was true only when ThisTDZMode was set to AlwaysCheck.
> +        ThisTDZMode was AlwaysCheck when we were a derived constructor.
> +        We already emit TDZ checks for 'this' when we're in a derived 
> +        constructor context. This field was doing nothing.

In Parser<LexerType>::Parser(), can you ASSERT(m_thisTDZMode == AlwaysCheck) when (derivedContextType == DerivedContextType::DerivedConstructorContext) to enforce this invariant?  Is that assert right?

Comment 3 Saam Barati 2016-04-29 16:21:10 PDT

Comment on attachment 277743 [details]
patch

I'm redoing this patch to remove ThisTDZMode since it isn't needed.

Comment 4 Saam Barati 2016-04-29 17:07:09 PDT

Created attachment 277760 [details]
patch

Comment 5 Mark Lam 2016-04-29 17:28:35 PDT

Comment on attachment 277760 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:12
> +        my removing ThisTDZMode.

typo: /my/by/.

> Source/JavaScriptCore/parser/SourceCodeKey.h:47
> -    SourceCodeKey(const SourceCode& sourceCode, const String& name, CodeType codeType, JSParserBuiltinMode builtinMode, JSParserStrictMode strictMode, ThisTDZMode thisTDZMode = ThisTDZMode::CheckIfNeeded)
> +    SourceCodeKey(const SourceCode& sourceCode, const String& name, CodeType codeType, JSParserBuiltinMode builtinMode, JSParserStrictMode strictMode)
>          : m_sourceCode(sourceCode)
>          , m_name(name)
> -        , m_flags((static_cast<unsigned>(codeType) << 3) | (static_cast<unsigned>(builtinMode) << 2) | (static_cast<unsigned>(strictMode) << 1) | static_cast<unsigned>(thisTDZMode))
> +        , m_flags((static_cast<unsigned>(codeType) << 2) | (static_cast<unsigned>(builtinMode) << 1) | static_cast<unsigned>(strictMode))

Are you sure that we no longer need thisTDZMode (or some equivalent) in the SourceCodeKey?  This elision of thisTDZMode here is a change of behavior because SourceCodeKey does not know about ConstructorKind and DerivedContextType.  From what I can tell, JSGlobalObject::createEvalCodeBlock() eventually calls CodeCache::getGlobalCodeBlock(), and will have different behavior depending on whether thisTDZMode.

So, is this actually correct?

Comment 6 Mark Lam 2016-04-29 17:42:55 PDT

Comment on attachment 277760 [details]
patch

r- for to take this out of the review queue, until the SourceCodeKey issue is addressed.

Comment 7 Saam Barati 2016-04-30 00:36:24 PDT

Comment on attachment 277760 [details]
patch

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

>> Source/JavaScriptCore/parser/SourceCodeKey.h:47
>> +        , m_flags((static_cast<unsigned>(codeType) << 2) | (static_cast<unsigned>(builtinMode) << 1) | static_cast<unsigned>(strictMode))
> 
> Are you sure that we no longer need thisTDZMode (or some equivalent) in the SourceCodeKey?  This elision of thisTDZMode here is a change of behavior because SourceCodeKey does not know about ConstructorKind and DerivedContextType.  From what I can tell, JSGlobalObject::createEvalCodeBlock() eventually calls CodeCache::getGlobalCodeBlock(), and will have different behavior depending on whether thisTDZMode.
> 
> So, is this actually correct?

I think the correct solution here is to take into account DerivedContextType and EvalContextType in SourceCodeKey.
I can see how this might cause us to do bad things by not taking this into account. It's not super obvious which situations bad
things would happen in by not taking these fields into account. I think we would probably cache an eval inside a function
and outside a function to each other even though they have different context types. (This effects new.target, but I'm not sure
what else).

It's obvious that we should just take into account those fields here. If they're the same, then obviously they can safely cache.

I'll make this change and re-upload.

Comment 8 Yusuke Suzuki 2016-05-15 16:51:02 PDT

Discussed with Saam. We should fix the same issue for EvalCodeCache.

Comment 9 Yusuke Suzuki 2016-05-22 07:30:51 PDT

Created attachment 279549 [details]
Patch

Comment 10 Yusuke Suzuki 2016-05-22 07:38:57 PDT

Comment on attachment 279549 [details]
Patch

Hm, we can see date-format-xparb regression...

Comment 11 Yusuke Suzuki 2016-05-22 09:26:51 PDT

Ah, it seems that was noise.

Benchmark report for SunSpider on hanayamata.

VMs tested:
"baseline" at /home/yusukesuzuki/dev/WebKit/WebKitBuild/unify-master/Release/bin/jsc
"patched" at /home/yusukesuzuki/dev/WebKit/WebKitBuild/unify/Release/bin/jsc

Collected 50 samples per benchmark/VM, with 50 VM invocations per benchmark. Emitted a call to gc()
between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the
jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark execution times
with 95% confidence intervals in milliseconds.

                                   baseline                  patched                                      

3d-cube                         6.0384+-0.0133     ?      6.0613+-0.0152        ?
3d-morph                       25.8352+-0.0615     ?     25.8728+-0.1559        ?
3d-raytrace                     7.0023+-0.2495     ?      7.0058+-0.2359        ?
access-binary-trees             2.3516+-0.0751     ?      2.3631+-0.0731        ?
access-fannkuch                 6.9495+-0.2242     ?      7.0143+-0.2411        ?
access-nbody                    2.8772+-0.0113            2.8732+-0.0093        
access-nsieve                   3.1959+-0.0329            3.1883+-0.0315        
bitops-3bit-bits-in-byte        1.2477+-0.0387            1.2139+-0.0305          might be 1.0278x faster
bitops-bits-in-byte             2.8216+-0.0782            2.7863+-0.0768          might be 1.0127x faster
bitops-bitwise-and              1.9834+-0.0072     ?      1.9881+-0.0082        ?
bitops-nsieve-bits              3.2078+-0.0803            3.1746+-0.0716          might be 1.0105x faster
controlflow-recursive           2.8659+-0.0805            2.7317+-0.0754          might be 1.0491x faster
crypto-aes                      5.2269+-0.0184     ?      5.2497+-0.0179        ?
crypto-md5                      2.8358+-0.1051     ?      2.9040+-0.1196        ? might be 1.0240x slower
crypto-sha1                     2.6197+-0.0551     ?      2.6537+-0.0593        ? might be 1.0130x slower
date-format-tofte              11.1936+-0.1342           11.1609+-0.1325        
date-format-xparb               6.1710+-0.0790     ?      6.2312+-0.0625        ?
math-cordic                     3.0773+-0.0579     ?      3.1374+-0.0923        ? might be 1.0195x slower
math-partial-sums              10.4776+-0.0119     ?     10.4992+-0.0159        ?
math-spectral-norm              2.1895+-0.0359     ?      2.1961+-0.0391        ?
regexp-dna                      7.3546+-0.0206            7.3518+-0.0206        
string-base64                   4.1672+-0.0236            4.1476+-0.0271        
string-fasta                    6.3762+-0.1015     ?      6.5688+-0.1775        ? might be 1.0302x slower
string-tagcloud                 9.6605+-0.0378     ?      9.6630+-0.0848        ?
string-unpack-code             20.1848+-0.2130           19.9696+-0.1192          might be 1.0108x faster
string-validate-input           4.3347+-0.0585            4.3259+-0.0656        

<arithmetic>                    6.2402+-0.0188     ?      6.2436+-0.0206        ? might be 1.0005x slower

Comment 12 Saam Barati 2016-05-23 21:54:48 PDT

Comment on attachment 279549 [details]
Patch

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

r=me
Make sure to run JSBench and see that there aren't regressions there because it's a good benchmark for code caching.

> Source/JavaScriptCore/runtime/CodeCache.h:71
> +    typedef SourceCodeKey Key;

I don't think we really need this typedef it seems to only

> Source/JavaScriptCore/runtime/CodeCache.h:72
> +    typedef HashMap<Key, SourceCodeValue, Key::Hash, Key::HashTraits> MapType;

result in a loss of information. 
(Sorry for weird editing I'm on an iPhone).

Comment 13 Yusuke Suzuki 2016-05-24 03:29:03 PDT

Comment on attachment 279549 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/runtime/CodeCache.h:71
>> +    typedef SourceCodeKey Key;
> 
> I don't think we really need this typedef it seems to only

Ah, this is the change caused when I used SourceCodeKey<Owner> in some revisions. (And share SourceCodeKey<...> impl in EvalCodeCache / CodeCache. But it causes 1 % regression in date-format-tofte due to String for name etc.)
Dropped. Thanks.

Comment 14 Yusuke Suzuki 2016-05-24 03:30:49 PDT

I've just taken the JSBench results in GTK port.
With https://bugs.webkit.org/show_bug.cgi?id=157952 patch and some modification in run-jsc-benchmark.
But I'll take the results in OS X soon.

Seeing the JSBench, since it uses (new Date()).getTime(), the result becomes milliseconds. I think it's worth fixing in the other bugs.


Generating benchmark report at /home/yusukesuzuki/dev/WebKit/baseline_patched_JSBench_hanayamata_20160524_1902_report.txt
And raw data at /home/yusukesuzuki/dev/WebKit/baseline_patched_JSBench_hanayamata_20160524_1902.json

Benchmark report for JSBench on hanayamata.

VMs tested:
"baseline" at /home/yusukesuzuki/dev/WebKit/WebKitBuild/unify-master/Release/bin/WebKitTestRunner
"patched" at /home/yusukesuzuki/dev/WebKit/WebKitBuild/unify/Release/bin/WebKitTestRunner

Collected 10 samples per benchmark/VM, with 10 VM invocations per benchmark. Emitted a call to gc()
between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the
jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark execution
times with 95% confidence intervals in milliseconds.

                               baseline                  patched                                      

amazon-chrome               4.7000+-0.3456            4.7000+-0.3456        
amazon-chrome-win           5.0000+-0.0000     ?      5.1000+-0.2262        ? might be 1.0200x slower
amazon-firefox              4.3000+-0.3456            4.1000+-0.2262          might be 1.0488x faster
amazon-firefox-win          4.1000+-0.2262            4.1000+-0.2262        
amazon-safari               5.3000+-0.3456            5.2000+-0.3016          might be 1.0192x faster
facebook-chrome            60.6000+-1.7261     ?     60.7000+-2.1073        ?
facebook-chrome-win       110.0000+-4.9100          108.0000+-3.6163          might be 1.0185x faster
facebook-firefox           32.4000+-0.3694           32.4000+-0.3694        
facebook-firefox-win       21.2000+-0.3016           21.0000+-0.0000        
facebook-safari           108.4000+-0.7690     ?    108.5000+-1.9151        ?
google-chrome              54.7000+-2.8425           54.0000+-2.1060          might be 1.0130x faster
google-chrome-win          43.1000+-1.5250     ?     43.5000+-1.9151        ?
google-firefox             22.4000+-0.9049           21.9000+-0.2262          might be 1.0228x faster
google-firefox-win         25.9000+-0.2262           25.7000+-0.3456        
google-safari              45.9000+-0.2262     ?     46.3000+-0.8954        ?
twitter-chrome              8.0000+-0.3372            7.8000+-0.3016          might be 1.0256x faster
twitter-chrome-win          7.7000+-0.3456     ?      7.9000+-0.4061        ? might be 1.0260x slower
twitter-firefox             0.9000+-0.2262            0.9000+-0.2262        
twitter-firefox-win         0.9000+-0.2262     ?      1.0000+-0.0000        ? might be 1.1111x slower
twitter-safari              7.4000+-0.3694            7.4000+-0.3694        
yahoo-chrome               46.6000+-2.4130           45.4000+-0.3694          might be 1.0264x faster
yahoo-chrome-win           42.6000+-2.1645           41.7000+-0.3456          might be 1.0216x faster
yahoo-firefox              45.6000+-0.3694           45.4000+-0.3694        
yahoo-firefox-win          32.4000+-1.2708     ?     32.5000+-1.4406        ?
yahoo-safari               45.7000+-0.4828           45.5000+-0.3770        

<arithmetic>               31.4320+-0.3642           31.2280+-0.1754          might be 1.0065x faster

Comment 15 Yusuke Suzuki 2016-05-24 04:17:22 PDT

precise time version is pasted (GTK port). The performance result seems neutral.

Benchmark report for JSBench on hanayamata.

VMs tested:
"baseline" at /home/yusukesuzuki/dev/WebKit/WebKitBuild/unify-master/Release/bin/WebKitTestRunner
"patched" at /home/yusukesuzuki/dev/WebKit/WebKitBuild/unify/Release/bin/WebKitTestRunner

Collected 10 samples per benchmark/VM, with 10 VM invocations per benchmark. Emitted a call to gc()
between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the
jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark execution
times with 95% confidence intervals in milliseconds.

                               baseline                  patched                                      

amazon-chrome               4.9325+-0.0708            4.9228+-0.0323        
amazon-chrome-win           5.0518+-0.1095            4.9932+-0.0535          might be 1.0117x faster
amazon-firefox              4.2443+-0.0146     ?      4.2961+-0.0512        ? might be 1.0122x slower
amazon-firefox-win          4.0796+-0.0437            4.0083+-0.0349          might be 1.0178x faster
amazon-safari               5.2075+-0.0351     ?      5.2125+-0.0294        ?
facebook-chrome            61.8494+-2.3841           59.6961+-0.2352          might be 1.0361x faster
facebook-chrome-win       106.6280+-0.3246          106.1090+-0.3561        
facebook-firefox           32.4750+-0.1524           32.2546+-0.1619        
facebook-firefox-win       21.0284+-0.1064           20.9625+-0.0801        
facebook-safari           108.6636+-0.5265     ?    109.4860+-3.7007        ?
google-chrome              52.5803+-0.1479     ?     52.7782+-0.3385        ?
google-chrome-win          42.4969+-0.2176           42.3505+-0.1212        
google-firefox             21.9108+-0.0751           21.8922+-0.0805        
google-firefox-win         25.8621+-0.1576           25.7858+-0.1219        
google-safari              46.3099+-0.6741     ?     46.9237+-1.9326        ? might be 1.0133x slower
twitter-chrome              7.7145+-0.0452            7.6757+-0.0439        
twitter-chrome-win          7.7968+-0.0401            7.7501+-0.0354        
twitter-firefox             0.9288+-0.0111     ?      0.9358+-0.0115        ?
twitter-firefox-win         0.9150+-0.0109            0.9062+-0.0152        
twitter-safari              7.3029+-0.2249            7.1729+-0.0285          might be 1.0181x faster
yahoo-chrome               45.7975+-0.2063           45.3870+-0.3167        
yahoo-chrome-win           41.9409+-0.2908           41.9063+-0.8450        
yahoo-firefox              45.6259+-0.2709     ?     46.2049+-1.7923        ? might be 1.0127x slower
yahoo-firefox-win          31.9043+-0.2250     ?     32.2298+-1.5957        ? might be 1.0102x slower
yahoo-safari               45.7692+-0.2300           45.3233+-0.2835        

<arithmetic>               31.1606+-0.0879           31.0865+-0.1758          might be 1.0024x faster

Comment 16 Yusuke Suzuki 2016-05-24 04:56:56 PDT

Seems OK in OS X port. (https://bugs.webkit.org/show_bug.cgi?id=157952 with precision time change)

Benchmark report for JSBench on dandelion (MacBookPro10,1).

VMs tested:
"baseline" at /Users/yusukesuzuki/dev/WebKit/WebKitBuild/unify-master/Release/WebKitTestRunner
"patched" at /Users/yusukesuzuki/dev/WebKit/WebKitBuild/unify/Release/WebKitTestRunner

Collected 10 samples per benchmark/VM, with 10 VM invocations per benchmark. Emitted a call to gc()
between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the
jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark execution
times with 95% confidence intervals in milliseconds.

                               baseline                  patched                                      

amazon-chrome               5.4971+-0.3937            5.2101+-0.1327          might be 1.0551x faster
amazon-chrome-win           5.4426+-0.3389            5.2734+-0.0883          might be 1.0321x faster
amazon-firefox              4.6832+-0.1219     ?      4.9417+-0.3392        ? might be 1.0552x slower
amazon-firefox-win          4.6663+-0.3976            4.6185+-0.3174          might be 1.0104x faster
amazon-safari               5.5309+-0.3497     ?      5.6208+-0.3330        ? might be 1.0163x slower
facebook-chrome            58.9424+-0.7148           58.0706+-1.3325          might be 1.0150x faster
facebook-chrome-win       102.3439+-0.9822     ?    102.6129+-1.6261        ?
facebook-firefox           30.5548+-0.8753     ?     30.5930+-0.6956        ?
facebook-firefox-win       21.4658+-0.4832           21.4091+-0.7342        
facebook-safari           106.0003+-1.4233          104.8750+-1.4268          might be 1.0107x faster
google-chrome              50.7902+-0.9567     ?     50.9775+-1.0482        ?
google-chrome-win          42.7208+-0.9818           41.7203+-1.0045          might be 1.0240x faster
google-firefox             22.1858+-0.6207     ?     22.8521+-0.6021        ? might be 1.0300x slower
google-firefox-win         26.6523+-0.8937     ?     27.1894+-0.9687        ? might be 1.0202x slower
google-safari              45.4453+-1.5787     ?     45.7893+-1.6880        ?
twitter-chrome              7.5743+-0.4902     ?      7.5935+-0.4605        ?
twitter-chrome-win          8.0579+-0.6966     ?      8.2815+-0.7664        ? might be 1.0278x slower
twitter-firefox             1.0250+-0.0669     ?      1.0379+-0.0566        ? might be 1.0126x slower
twitter-firefox-win         0.9414+-0.0501     ?      0.9707+-0.0614        ? might be 1.0311x slower
twitter-safari              7.7673+-0.7064            7.5038+-0.6054          might be 1.0351x faster
yahoo-chrome               43.2073+-0.8112     ?     44.2817+-1.8422        ? might be 1.0249x slower
yahoo-chrome-win           39.5138+-0.8290           39.2345+-0.8166        
yahoo-firefox              44.4406+-1.3704           43.8516+-1.2818          might be 1.0134x faster
yahoo-firefox-win          30.7475+-0.6733           30.7448+-0.9187        
yahoo-safari               44.1054+-1.3421     ?     44.3163+-1.5547        ?

<arithmetic>               30.4121+-0.2557           30.3828+-0.2540          might be 1.0010x faster

Comment 17 Yusuke Suzuki 2016-05-24 05:03:32 PDT

Committed r201328: <http://trac.webkit.org/changeset/201328>