[JSC] DFG::Node should not have its own allocator
Created attachment 284371 [details] Patch
Comment on attachment 284371 [details] Patch Uh, 32bits does not build B3.
Created attachment 284383 [details] Patch
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?
It would also be interesting to see various compile time comparisons and not just benchmarks.
> 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).
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.
> 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.
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
(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.
> > 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.
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.
Comment on attachment 284383 [details] Patch Clearing flags on attachment: 284383 Committed r203703: <http://trac.webkit.org/changeset/203703>
All reviewed patches have been landed. Closing bug.
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
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.
Reverted r203703 for reason: It breaks some internal tests Committed r203704: <http://trac.webkit.org/changeset/203704>
Created attachment 284747 [details] Patch
Comment on attachment 284747 [details] Patch Second attempt.
Comment on attachment 284747 [details] Patch Clearing flags on attachment: 284747 Committed r203808: <http://trac.webkit.org/changeset/203808>
Looks like this might be a 1% JetStream regression on Mac.
Rolled out in https://bugs.webkit.org/show_bug.cgi?id=160784.
Reopening to attach new patch.
Created attachment 309948 [details] Patch
(In reply to Geoffrey Garen from comment #25) > Created attachment 309948 [details] > Patch Are perf results neutral now?
(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.
Created attachment 309957 [details] Patch
Created attachment 309976 [details] Patch
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.
(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.
> > 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.
(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.
> 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!
Committed r216815: <http://trac.webkit.org/changeset/216815>
MacBook Pro JetStream bots say this is fine. MacBook Air JetStream bots haven't cycled yet.
iOS JetStream bots say this change is OK.
MacBook Air bot says no regression.