Bug 160098

Summary:

[JSC] DFG::Node should not have its own allocator

Product:

WebKit

Reporter:

Benjamin Poulain <benjamin>

Component:

New Bugs

Assignee:

Geoffrey Garen <ggaren>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, commit-queue, fpizlo, ggaren, keith_miller, mark.lam, msaboff, ossy, saam

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

160228

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	saam: review+
Patch	saam: review+

Benjamin Poulain

Reported 2016-07-22 14:05:11 PDT

[JSC] DFG::Node should not have its own allocator

Attachments
Patch (49.50 KB, patch) 2016-07-22 14:09 PDT, Benjamin Poulain	no flags	Details Formatted Diff Diff
Patch (50.08 KB, patch) 2016-07-22 16:03 PDT, Benjamin Poulain	no flags	Details Formatted Diff Diff
Patch (50.21 KB, patch) 2016-07-27 17:34 PDT, Benjamin Poulain	no flags	Details Formatted Diff Diff
Patch (36.97 KB, patch) 2017-05-12 14:56 PDT, Geoffrey Garen	no flags	Details Formatted Diff Diff
Patch (37.21 KB, patch) 2017-05-12 15:47 PDT, Geoffrey Garen	saam: review+	Details Formatted Diff Diff
Patch (37.71 KB, patch) 2017-05-12 17:36 PDT, Geoffrey Garen	saam: review+	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Benjamin Poulain

Comment 1 2016-07-22 14:09:13 PDT

Created attachment 284371 [details] Patch

Benjamin Poulain

Comment 2 2016-07-22 15:35:02 PDT

Comment on attachment 284371 [details] Patch Uh, 32bits does not build B3.

Benjamin Poulain

Comment 3 2016-07-22 16:03:34 PDT

Created attachment 284383 [details] Patch

Geoffrey Garen

Comment 4 2016-07-24 09:21:49 PDT

Comment on attachment 284383 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=284383&action=review r=me Do we have performance results for this change? I think we need that before landing. > Source/JavaScriptCore/dfg/DFGNode.h:2351 > + unsigned m_index { std::numeric_limits<unsigned>::max() }; Where does this value get initialized now?

Saam Barati

Comment 5 2016-07-24 09:42:21 PDT

It would also be interesting to see various compile time comparisons and not just benchmarks.

Geoffrey Garen

Comment 6 2016-07-24 18:29:48 PDT

> It would also be interesting to see various compile time comparisons and not > just benchmarks. I agree -- though I'll also point out that only an end-to-end benchmark can fully weigh the advantages and disadvantages of private slab allocation (the status quo) vs shared heap (this patch).

Csaba Osztrogonác

Comment 7 2016-07-25 02:04:43 PDT

Comment on attachment 284383 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=284383&action=review > Source/JavaScriptCore/b3/B3SparseCollection.h:-30 > -#if ENABLE(B3_JIT) > - It's so ugly to use a B3 file outside B3 directory. I suggest you should move it outside of B3 and remove the B3 prefix from the filename and namespace too.

Geoffrey Garen

Comment 8 2016-07-25 10:51:09 PDT

> I suggest you should move it outside of B3 and remove > the B3 prefix from the filename and namespace too. Yeah, this could just bet WTF::SparseCollection.

Benjamin Poulain

Comment 9 2016-07-25 16:49:57 PDT

This is just a prerequisite for the next changes. No perf improvement intended here. Bench results: Conf#1 Conf#2 SunSpider: 3d-cube 4.5836+-0.3397 ? 4.7505+-0.4177 ? might be 1.0364x slower 3d-morph 4.9816+-0.1035 4.9611+-0.1475 3d-raytrace 4.9725+-0.1169 ? 5.0789+-0.3238 ? might be 1.0214x slower access-binary-trees 1.9615+-0.0437 ? 2.0272+-0.2319 ? might be 1.0335x slower access-fannkuch 5.8403+-0.3337 5.6096+-0.1484 might be 1.0411x faster access-nbody 2.5463+-0.1532 2.5067+-0.1230 might be 1.0158x faster access-nsieve 3.0684+-0.1134 2.9572+-0.1023 might be 1.0376x faster bitops-3bit-bits-in-byte 1.0390+-0.0272 ? 1.0470+-0.0284 ? bitops-bits-in-byte 2.5625+-0.0576 ? 2.6500+-0.0988 ? might be 1.0342x slower bitops-bitwise-and 1.9964+-0.1658 1.9807+-0.0446 bitops-nsieve-bits 3.3705+-0.5680 3.1161+-0.0246 might be 1.0816x faster controlflow-recursive 2.2792+-0.0300 ? 2.2967+-0.0464 ? crypto-aes 4.3719+-0.0925 ? 4.5295+-0.0864 ? might be 1.0360x slower crypto-md5 2.6827+-0.0983 2.6353+-0.1061 might be 1.0180x faster crypto-sha1 2.7142+-0.0765 ? 2.7793+-0.1252 ? might be 1.0240x slower date-format-tofte 6.3802+-0.0477 6.3069+-0.1799 might be 1.0116x faster date-format-xparb 4.7271+-0.1744 ? 4.7435+-0.1847 ? math-cordic 2.7368+-0.0930 ? 3.2941+-1.8430 ? might be 1.2036x slower math-partial-sums 3.9833+-0.0799 ? 4.0600+-0.1291 ? might be 1.0193x slower math-spectral-norm 1.9470+-0.0211 ? 1.9800+-0.0715 ? might be 1.0170x slower regexp-dna 6.3972+-0.3792 ? 6.5093+-0.3067 ? might be 1.0175x slower string-base64 4.0341+-0.0622 ? 4.0421+-0.0420 ? string-fasta 5.3906+-0.0500 ? 5.4359+-0.0625 ? string-tagcloud 8.3417+-0.2300 8.2700+-0.1057 string-unpack-code 18.4478+-0.9387 18.2593+-1.0403 might be 1.0103x faster string-validate-input 4.0724+-0.1434 ? 4.0825+-0.1460 ? <arithmetic> 4.4396+-0.0598 ? 4.4581+-0.1155 ? might be 1.0042x slower Conf#1 Conf#2 LongSpider: 3d-cube 814.1968+-13.9080 803.0479+-6.5301 might be 1.0139x faster 3d-morph 595.2142+-1.6293 ? 595.8592+-1.9296 ? 3d-raytrace 477.0824+-13.0346 ? 481.0647+-15.5629 ? access-binary-trees 824.0523+-2.4855 821.3245+-0.6398 access-fannkuch 244.6966+-1.8342 ? 247.5770+-12.5992 ? might be 1.0118x slower access-nbody 547.5306+-7.5478 537.7537+-17.9482 might be 1.0182x faster access-nsieve 290.3370+-19.0784 ? 295.2606+-27.1267 ? might be 1.0170x slower bitops-3bit-bits-in-byte 32.7778+-0.0973 ? 32.8211+-0.2815 ? bitops-bits-in-byte 99.1688+-1.0427 ? 100.7849+-3.6052 ? might be 1.0163x slower bitops-nsieve-bits 390.1149+-9.4305 387.0555+-1.3148 controlflow-recursive 461.2272+-1.1946 ? 462.8629+-4.7241 ? crypto-aes 580.6058+-4.5551 ? 581.4628+-2.2363 ? crypto-md5 488.4390+-7.5242 ! 501.9428+-5.3408 ! definitely 1.0276x slower crypto-sha1 655.9058+-10.7790 ? 657.3202+-14.6412 ? date-format-tofte 336.7000+-2.9436 336.3047+-6.0693 date-format-xparb 631.8662+-6.4577 626.9180+-11.9064 hash-map 144.2493+-9.5339 ? 147.1482+-3.8239 ? might be 1.0201x slower math-cordic 444.9510+-10.5592 442.6737+-9.6940 math-partial-sums 284.0110+-0.7932 283.5725+-0.5594 math-spectral-norm 552.3792+-4.1721 ? 557.6889+-6.0465 ? string-base64 344.7924+-3.7015 343.2288+-1.4431 string-fasta 335.7288+-9.1996 ? 338.4246+-6.2538 ? string-tagcloud 174.2877+-6.1980 172.9207+-2.1834 <geometric> 350.9745+-2.4860 ? 351.8233+-1.9755 ? might be 1.0024x slower Conf#1 Conf#2 V8Spider: crypto 36.0634+-0.4051 ? 36.3123+-0.4496 ? deltablue 49.6981+-1.1390 ? 52.3143+-3.3507 ? might be 1.0526x slower earley-boyer 39.0964+-0.4292 ? 39.7713+-0.7411 ? might be 1.0173x slower raytrace 20.6555+-0.7178 ? 20.8365+-0.2198 ? regexp 53.0681+-1.6381 ? 53.6110+-2.0561 ? might be 1.0102x slower richards 41.0083+-1.4894 40.6258+-0.6122 splay 29.5540+-1.1945 29.4339+-0.9263 <geometric> 36.8926+-0.3953 ? 37.3195+-0.3886 ? might be 1.0116x slower Conf#1 Conf#2 Octane: encrypt 0.15935+-0.00206 ? 0.15949+-0.00450 ? decrypt 2.68049+-0.01307 2.67917+-0.01530 deltablue x2 0.13047+-0.00561 ? 0.13051+-0.00296 ? earley 0.28515+-0.00227 ? 0.28687+-0.00312 ? boyer 4.98511+-0.06987 ? 5.00160+-0.04835 ? navier-stokes x2 4.92439+-0.02021 ? 4.95331+-0.03256 ? raytrace x2 0.80400+-0.00850 ? 0.80712+-0.01610 ? richards x2 0.08147+-0.00185 ? 0.08306+-0.00479 ? might be 1.0195x slower splay x2 0.33811+-0.00108 ? 0.34345+-0.01939 ? might be 1.0158x slower regexp x2 16.08042+-0.86763 ? 16.22920+-1.31226 ? pdfjs x2 38.42773+-0.62616 ? 38.54145+-0.44634 ? mandreel x2 42.77889+-0.40898 ? 42.94046+-1.00047 ? gbemu x2 29.65128+-0.12255 29.45333+-0.38738 closure 0.48247+-0.00431 0.48182+-0.00337 jquery 6.36401+-0.03640 ? 6.37159+-0.07905 ? box2d x2 9.22148+-0.08685 ? 9.31903+-0.06357 ? might be 1.0106x slower zlib x2 358.16150+-22.25503 ? 367.97878+-5.02124 ? might be 1.0274x slower typescript x2 581.85010+-32.24668 ? 602.52795+-6.35020 ? might be 1.0355x slower <geometric> 4.98543+-0.03715 ? 5.02923+-0.03750 ? might be 1.0088x slower Conf#1 Conf#2 Kraken: ai-astar 87.559+-5.623 85.709+-1.477 might be 1.0216x faster audio-beat-detection 38.719+-0.240 38.693+-0.321 audio-dft 96.634+-1.431 ? 96.808+-1.089 ? audio-fft 30.284+-0.096 30.278+-0.051 audio-oscillator 48.754+-0.244 ^ 47.652+-0.332 ^ definitely 1.0231x faster imaging-darkroom 61.081+-0.125 ? 61.123+-0.321 ? imaging-desaturate 43.984+-0.177 ? 44.689+-2.543 ? might be 1.0160x slower imaging-gaussian-blur 62.499+-3.068 60.988+-3.018 might be 1.0248x faster json-parse-financial 35.986+-3.223 34.652+-2.098 might be 1.0385x faster json-stringify-tinderbox 24.043+-1.170 ? 24.629+-1.205 ? might be 1.0244x slower stanford-crypto-aes 36.704+-0.154 ? 36.857+-0.471 ? stanford-crypto-ccm 33.510+-1.267 ? 34.316+-5.406 ? might be 1.0240x slower stanford-crypto-pbkdf2 93.491+-4.126 92.750+-1.984 stanford-crypto-sha256-iterative 30.211+-0.463 30.107+-0.212 <arithmetic> 51.676+-1.004 51.375+-0.375 might be 1.0059x faster Conf#1 Conf#2 JSRegress: ... <geometric> 6.6183+-0.0098 6.6178+-0.0222 might be 1.0001x faster Conf#1 Conf#2 AsmBench: bigfib.cpp 426.2920+-12.8457 ? 426.4513+-2.9671 ? cray.c 384.1620+-2.9445 ? 387.1522+-4.2875 ? dry.c 482.7440+-163.5305 427.1642+-15.8615 might be 1.1301x faster FloatMM.c 714.7203+-8.7752 ? 721.1167+-13.1488 ? gcc-loops.cpp 3596.2211+-28.2532 ? 3661.3823+-59.3434 ? might be 1.0181x slower n-body.c 799.9656+-5.6063 ? 804.9960+-8.1356 ? Quicksort.c 400.3997+-14.2065 398.1345+-14.0623 stepanov_container.cpp 3262.5024+-28.7497 ? 3294.4413+-49.5728 ? Towers.c 264.0674+-1.0000 ? 264.3337+-0.9620 ? <geometric> 724.9118+-22.5246 719.9194+-5.9676 might be 1.0069x faster Conf#1 Conf#2 CompressionBench: huffman 33.1841+-1.5393 ! 36.0825+-0.9056 ! definitely 1.0873x slower arithmetic-simple 259.3812+-2.0594 258.4003+-2.5427 arithmetic-precise 243.7347+-4.8619 243.5977+-4.5461 arithmetic-complex-precise 247.0735+-9.2874 239.2947+-1.2824 might be 1.0325x faster arithmetic-precise-order-0 263.5537+-1.4464 263.2097+-6.0637 arithmetic-precise-order-1 303.0754+-2.7143 297.8614+-6.1711 might be 1.0175x faster arithmetic-precise-order-2 332.4570+-2.8310 ? 334.0301+-4.1523 ? arithmetic-simple-order-1 307.1030+-1.5720 306.6102+-3.8142 arithmetic-simple-order-2 358.2093+-3.3284 ? 362.6561+-17.5987 ? might be 1.0124x slower lz-string 276.9891+-2.9038 ? 277.2866+-5.7870 ? <geometric> 230.2706+-1.2441 ? 231.3146+-2.0397 ? might be 1.0045x slower Conf#1 Conf#2 Geomean of preferred means: <scaled-result> 44.8449+-0.1256 ? 44.9495+-0.1751 ? might be 1.0023x slower Fast benchmarks (20 times): Conf#1 Conf#2 SunSpider: 3d-cube 4.7572+-0.0686 4.6810+-0.0801 might be 1.0163x faster 3d-morph 5.2314+-0.3427 4.9305+-0.0258 might be 1.0610x faster 3d-raytrace 4.9503+-0.0243 ? 4.9645+-0.0476 ? access-binary-trees 1.9265+-0.0196 ? 1.9516+-0.0164 ? might be 1.0130x slower access-fannkuch 5.7562+-0.1228 5.6229+-0.0642 might be 1.0237x faster access-nbody 2.5011+-0.0211 2.4865+-0.0239 access-nsieve 3.0539+-0.0352 ^ 2.9335+-0.0282 ^ definitely 1.0410x faster bitops-3bit-bits-in-byte 1.0499+-0.0114 ? 1.0723+-0.0222 ? might be 1.0213x slower bitops-bits-in-byte 2.6300+-0.0715 2.5676+-0.0232 might be 1.0243x faster bitops-bitwise-and 1.9621+-0.0158 ? 2.0171+-0.0399 ? might be 1.0280x slower bitops-nsieve-bits 3.1489+-0.0882 3.1414+-0.0222 controlflow-recursive 2.3005+-0.0237 2.2949+-0.0166 crypto-aes 4.3848+-0.0415 ? 4.5537+-0.1458 ? might be 1.0385x slower crypto-md5 2.6661+-0.0289 2.6551+-0.0248 crypto-sha1 2.7099+-0.0230 ? 2.7916+-0.0645 ? might be 1.0302x slower date-format-tofte 6.3407+-0.0502 ? 6.3818+-0.0682 ? date-format-xparb 4.7178+-0.0521 ? 4.7404+-0.0950 ? math-cordic 2.7471+-0.0290 ? 2.7513+-0.0265 ? math-partial-sums 4.0329+-0.0630 ? 4.0995+-0.0740 ? might be 1.0165x slower math-spectral-norm 1.9965+-0.0306 ? 2.0066+-0.0265 ? regexp-dna 6.5255+-0.1465 6.4875+-0.0618 string-base64 4.1081+-0.0618 4.0572+-0.0323 might be 1.0126x faster string-fasta 5.4179+-0.0417 ? 5.4700+-0.0336 ? string-tagcloud 8.2939+-0.0525 8.2778+-0.0476 string-unpack-code 18.1401+-0.2992 17.7812+-0.2111 might be 1.0202x faster string-validate-input 4.0633+-0.0553 4.0373+-0.0188 <arithmetic> 4.4389+-0.0243 4.4136+-0.0153 might be 1.0057x faster Conf#1 Conf#2 Kraken: ai-astar 86.307+-0.724 ? 86.415+-0.795 ? audio-beat-detection 38.757+-0.130 38.690+-0.100 audio-dft 98.433+-1.472 ? 99.142+-1.525 ? audio-fft 30.227+-0.018 ? 30.429+-0.376 ? audio-oscillator 48.947+-0.169 ^ 47.713+-0.176 ^ definitely 1.0259x faster imaging-darkroom 61.503+-0.569 61.172+-0.098 imaging-desaturate 44.288+-0.509 44.084+-0.234 imaging-gaussian-blur 60.716+-0.887 ? 62.475+-1.685 ? might be 1.0290x slower json-parse-financial 34.954+-0.846 33.844+-0.477 might be 1.0328x faster json-stringify-tinderbox 24.147+-0.473 ? 24.309+-0.202 ? stanford-crypto-aes 37.123+-0.530 36.869+-0.137 stanford-crypto-ccm 33.390+-0.686 ? 34.120+-0.237 ? might be 1.0219x slower stanford-crypto-pbkdf2 92.535+-0.465 ? 92.683+-0.533 ? stanford-crypto-sha256-iterative 30.226+-0.102 ? 30.370+-0.203 ? <arithmetic> 51.539+-0.167 ? 51.594+-0.162 ? might be 1.0011x slower Conf#1 Conf#2 Geomean of preferred means: <scaled-result> 4.7830+-0.0142 4.7719+-0.0090 might be 1.0023x faster

Benjamin Poulain

Comment 10 2016-07-25 16:55:16 PDT

(In reply to comment #8) > > I suggest you should move it outside of B3 and remove > > the B3 prefix from the filename and namespace too. > > Yeah, this could just bet WTF::SparseCollection. Our layering has B3 under Air and DFG. I disagree SparseCollection should be in WTF given how specialized it is. I don't care enough to fight, I'll rename and put it somewhere to make you guys happy.

Geoffrey Garen

Comment 11 2016-07-25 16:59:43 PDT

> > Yeah, this could just bet WTF::SparseCollection. > > Our layering has B3 under Air and DFG. Yes, I agree. > I disagree SparseCollection should be in WTF given how specialized it is. OK -- let's not move it.

Benjamin Poulain

Comment 12 2016-07-25 17:07:05 PDT

Comment on attachment 284383 [details] Patch (In reply to comment #4) > > Source/JavaScriptCore/dfg/DFGNode.h:2351 > > + unsigned m_index { std::numeric_limits<unsigned>::max() }; > > Where does this value get initialized now? The SparseCollection has to be a friend of the node types. It sets the indices of its content. > > I disagree SparseCollection should be in WTF given how specialized it is. > > OK -- let's not move it. Ok, let's move forward then. We can always rename later.

WebKit Commit Bot

Comment 13 2016-07-25 17:28:59 PDT

Comment on attachment 284383 [details] Patch Clearing flags on attachment: 284383 Committed r203703: <http://trac.webkit.org/changeset/203703>

WebKit Commit Bot

Comment 14 2016-07-25 17:29:04 PDT

All reviewed patches have been landed. Closing bug.

Filip Pizlo

Comment 15 2016-07-25 17:45:35 PDT

I'm seeing crashes: Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x000000010b836cf7 bmalloc::Deallocator::processObjectLog(std::__1::lock_guard<bmalloc::StaticMutex>&) + 71 (SmallLine.h:63) 1 com.apple.JavaScriptCore 0x000000010b832dd7 bmalloc::Allocator::refillAllocatorSlowCase(bmalloc::BumpAllocator&, unsigned long) + 87 (atomic:848) 2 com.apple.JavaScriptCore 0x000000010b832f34 bmalloc::Allocator::allocateLogSizeClass(unsigned long) + 180 (BumpAllocator.h:76) 3 com.apple.JavaScriptCore 0x000000010b1271d0 JSC::B3::SparseCollection<JSC::DFG::Node>::remove(JSC::DFG::Node*) + 144 (Vector.h:269) 4 com.apple.JavaScriptCore 0x000000010b0de958 JSC::DFG::ConstantFoldingPhase::run() + 1096 (DFGConstantFoldingPhase.cpp:86) 5 com.apple.JavaScriptCore 0x000000010b0de3c9 JSC::DFG::performConstantFolding(JSC::DFG::Graph&) + 41 (DFGPhase.h:80) 6 com.apple.JavaScriptCore 0x000000010b1d8927 JSC::DFG::Plan::compileInThreadImpl() + 1287 (DFGPlan.cpp:411) 7 com.apple.JavaScriptCore 0x000000010b1d7e7d JSC::DFG::Plan::compileInThread(JSC::DFG::ThreadData*) + 589 (DFGPlan.cpp:189) 8 com.apple.JavaScriptCore 0x000000010b107507 JSC::DFG::compile(JSC::VM&, JSC::CodeBlock*, JSC::CodeBlock*, JSC::DFG::CompilationMode, unsigned int, JSC::Operands<JSC::JSValue, JSC::OperandValueTraits<JSC::JSValue> > const&, WTF::PassRefPtr<JSC::DeferredCompilationCallback>) + 1191 (DFGDriver.cpp:112) 9 com.apple.JavaScriptCore 0x000000010b1b647e JSC::DFG::triggerFTLReplacementCompile(JSC::VM*, JSC::CodeBlock*, JSC::DFG::JITCode*) + 558 (utility:753) 10 com.apple.JavaScriptCore 0x000000010b1b5dfd triggerTierUpNow + 301 (DFGOperations.cpp:1848) 11 ??? 0x00005d65df03518f 0 + 102692114616719 12 ??? 0x00005d65def6c09b 0 + 102692113793179 13 com.apple.JavaScriptCore 0x000000010b5b8b90 llint_entry + 25202 (LowLevelInterpreter.asm:733) 14 ??? 0x00005d65defe320e 0 + 102692114280974 15 ??? 0x00005d65def680a4 0 + 102692113776804 16 ??? 0x00005d65def0abd2 0 + 102692113394642 17 com.apple.JavaScriptCore 0x000000010b5b273b vmEntryToJavaScript + 299 (LowLevelInterpreter64.asm:255) 18 com.apple.JavaScriptCore 0x000000010b41b8fe JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 158 (JITCode.cpp:81) 19 com.apple.JavaScriptCore 0x000000010b3c34ff JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) + 16367 (Interpreter.cpp:962) 20 com.apple.JavaScriptCore 0x000000010b041735 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 469 (Completion.cpp:107) 21 jsc 0x000000010addb299 runJSC(JSC::VM*, CommandLine) + 2425 (jsc.cpp:2129) 22 jsc 0x000000010add9d7f jscmain(int, char**) + 575 (jsc.cpp:2431) 23 jsc 0x000000010add9aba main + 154 (jsc.cpp:2000) 24 libdyld.dylib 0x00007fffa4f83255 start + 1

Filip Pizlo

Comment 16 2016-07-25 17:53:08 PDT

Comment on attachment 284383 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=284383&action=review >>> Source/JavaScriptCore/dfg/DFGNode.h:2351 >>> + unsigned m_index { std::numeric_limits<unsigned>::max() }; >> >> Where does this value get initialized now? > > The SparseCollection has to be a friend of the node types. > It sets the indices of its content. This would read better as UINT_MAX.

Benjamin Poulain

Comment 17 2016-07-25 18:07:05 PDT

Reverted r203703 for reason: It breaks some internal tests Committed r203704: <http://trac.webkit.org/changeset/203704>

Benjamin Poulain

Comment 18 2016-07-27 17:34:57 PDT

Created attachment 284747 [details] Patch

Benjamin Poulain

Comment 19 2016-07-27 18:16:13 PDT

Comment on attachment 284747 [details] Patch Second attempt.

WebKit Commit Bot

Comment 20 2016-07-27 18:37:58 PDT

Comment on attachment 284747 [details] Patch Clearing flags on attachment: 284747 Committed r203808: <http://trac.webkit.org/changeset/203808>

WebKit Commit Bot

Comment 21 2016-07-27 18:38:03 PDT

All reviewed patches have been landed. Closing bug.

Saam Barati

Comment 22 2016-07-28 11:41:31 PDT

Looks like this might be a 1% JetStream regression on Mac.

Geoffrey Garen

Comment 23 2017-05-12 14:32:02 PDT

Rolled out in https://bugs.webkit.org/show_bug.cgi?id=160784.

Geoffrey Garen

Comment 24 2017-05-12 14:56:45 PDT

Reopening to attach new patch.

Geoffrey Garen

Comment 25 2017-05-12 14:56:46 PDT

Created attachment 309948 [details] Patch

Saam Barati

Comment 26 2017-05-12 15:18:33 PDT

(In reply to Geoffrey Garen from comment #25) > Created attachment 309948 [details] > Patch Are perf results neutral now?

Saam Barati

Comment 27 2017-05-12 15:19:02 PDT

(In reply to Saam Barati from comment #26) > (In reply to Geoffrey Garen from comment #25) > > Created attachment 309948 [details] > > Patch > > Are perf results neutral now? Oops, just saw your changelog. It appears so.

Geoffrey Garen

Comment 28 2017-05-12 15:47:35 PDT

Created attachment 309957 [details] Patch

Geoffrey Garen

Comment 29 2017-05-12 17:36:01 PDT

Created attachment 309976 [details] Patch

Saam Barati

Comment 30 2017-05-12 17:37:59 PDT

Comment on attachment 309957 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309957&action=review r=me You need to up the CMake lists file > Source/JavaScriptCore/dfg/DFGGraph.h:194 > Node* addNode(SpeculatedType type, Params... params) > { > - Node* node = new (m_allocator) Node(params...); > + Node* node = new Node(params...); Maybe it's worth changing this and the other variadic constructors to use perfect forwarding > Source/JavaScriptCore/dfg/DFGGraph.h:196 > + m_nodes.add(std::unique_ptr<Node>(node)); nit: I think addNew will do the making of the unique ptr for you.

Saam Barati

Comment 31 2017-05-12 17:38:34 PDT

(In reply to Saam Barati from comment #30) > Comment on attachment 309957 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=309957&action=review > > r=me > You need to up the CMake lists file Oops, I reviewed the old patch. > > > Source/JavaScriptCore/dfg/DFGGraph.h:194 > > Node* addNode(SpeculatedType type, Params... params) > > { > > - Node* node = new (m_allocator) Node(params...); > > + Node* node = new Node(params...); > > Maybe it's worth changing this and the other variadic constructors to use > perfect forwarding > > > Source/JavaScriptCore/dfg/DFGGraph.h:196 > > + m_nodes.add(std::unique_ptr<Node>(node)); > > nit: I think addNew will do the making of the unique ptr for you.

Geoffrey Garen

Comment 32 2017-05-12 17:45:14 PDT

> > Source/JavaScriptCore/dfg/DFGGraph.h:196 > > + m_nodes.add(std::unique_ptr<Node>(node)); > > nit: I think addNew will do the making of the unique ptr for you. I don't think you can rely on automatic construction because std::unique_ptr has an explicit constructor. I think that's a good thing because you really want the caller to indicate explicitly that it's giving away ownership.

Saam Barati

Comment 33 2017-05-12 18:10:41 PDT

(In reply to Geoffrey Garen from comment #32) > > > Source/JavaScriptCore/dfg/DFGGraph.h:196 > > > + m_nodes.add(std::unique_ptr<Node>(node)); > > > > nit: I think addNew will do the making of the unique ptr for you. > > I don't think you can rely on automatic construction because std::unique_ptr > has an explicit constructor. I think that's a good thing because you really > want the caller to indicate explicitly that it's giving away ownership. I'm just saying addNew is a function on sparse collection data structure you're using, and it just does the same thing you're doing. So you could just forward arguments to addNew instead of doing it yourself. It's not really a big deal either way.

Geoffrey Garen

Comment 34 2017-05-12 21:43:42 PDT

> I'm just saying addNew is a function on sparse collection data structure > you're using, and it just does the same thing you're doing. So you could > just forward arguments to addNew instead of doing it yourself. Oh! OK!

Geoffrey Garen

Comment 35 2017-05-12 21:59:37 PDT

Committed r216815: <http://trac.webkit.org/changeset/216815>

Geoffrey Garen

Comment 36 2017-05-13 09:17:15 PDT

MacBook Pro JetStream bots say this is fine. MacBook Air JetStream bots haven't cycled yet.

Geoffrey Garen

Comment 37 2017-05-13 09:19:40 PDT

iOS JetStream bots say this change is OK.

Geoffrey Garen

Comment 38 2017-05-13 11:37:15 PDT

MacBook Air bot says no regression.

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