Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2008043006:
[wasm] separate snapshot-able stages (Closed)
|
Created:
4 years, 7 months ago by Mircea Trofin Modified:
4 years, 6 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@refactoring Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[wasm] separate snapshot-able stages
This CLprepares the terrain for serialization/deserialization. It sets up
the instantiation stages such that we have a point wereh we can split off
obtaining the code from a snapshot, or snapshot. That point is after we
compile and produce the code table, but before we attach the
deoptimization info we use for stack tracing.
Opportunistically, performed more cleanup to improve maintainability:
- clarified sequential vs parallel compilation stages. FinishCompilation
was somewhat ambiguous in that it performed a few responsibilities:
compiling functions in the sequential case, and then populating the
linker and code tables.
- removed the "results" set, which is unnecessary. The linker simply
shares the function_code vector, and so do the compilation stages.
- populate the code table fixed array separately from compilation. This
falls out of the decisions above.
BUG=
Committed: https://crrev.com/3d25ad4df19a75010b120967d9ef3d589979e25c
Cr-Commit-Position: refs/heads/master@{#36618}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 9
Patch Set 4 : #Patch Set 5 : #
Total comments: 6
Patch Set 6 : #Patch Set 7 : #
Messages
Total messages: 23 (11 generated)
Description was changed from ========== fixups more refactoring BUG= ========== to ========== [wasm] separate snapshot-able stages This CLprepares the terrain for serialization/deserialization. It sets up the instantiation stages such that we have a point wereh we can split off obtaining the code from a snapshot, or snapshot. That point is after we compile and produce the code table, but before we attach the deoptimization info we use for stack tracing. Opportunistically, performed more cleanup to improve maintainability: - clarified sequential vs parallel compilation stages. FinishCompilation was somewhat ambiguous in that it performed a few responsibilities: compiling functions in the sequential case, and then populating the linker and code tables. - removed the "results" set, which is unnecessary. The linker simply shares the function_code vector, and so do the compilation stages. - populate the code table fixed array separately from compilation. This falls out of the decisions above. BUG= ==========
mtrofin@chromium.org changed reviewers: + bradnelson@chromium.org, titzer@chromium.org
https://codereview.chromium.org/2008043006/diff/40001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (left): https://codereview.chromium.org/2008043006/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:423: // TODO(ahaas): Maybe we could skip this for external functions. Why did you delete this code? It's necessary to create the placeholder objects before beginning parallel compilation. https://codereview.chromium.org/2008043006/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:505: if (FLAG_wasm_num_compilation_tasks != 0) { wat https://codereview.chromium.org/2008043006/diff/40001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2008043006/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:483: void CompileSequentially(Isolate* isolate, WasmModuleInstance* instance, I like the old name better. https://codereview.chromium.org/2008043006/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:605: void WasmModuleInstance::ProcessFunctionsPostLinking() { Please name this function for what it does. SetDeoptimizationData(), e.g.
https://codereview.chromium.org/2008043006/diff/40001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (left): https://codereview.chromium.org/2008043006/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:423: // TODO(ahaas): Maybe we could skip this for external functions. On 2016/05/26 15:12:25, titzer wrote: > Why did you delete this code? It's necessary to create the placeholder objects > before beginning parallel compilation. The CL has the refactoring one as a dependency. There, I moved this in a separate initialization call, just that I forgot to delete it from here. https://codereview.chromium.org/2008043006/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:505: if (FLAG_wasm_num_compilation_tasks != 0) { On 2016/05/26 15:12:25, titzer wrote: > wat Typo? https://codereview.chromium.org/2008043006/diff/40001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2008043006/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:483: void CompileSequentially(Isolate* isolate, WasmModuleInstance* instance, On 2016/05/26 15:12:25, titzer wrote: > I like the old name better. It doesn't finish anything anymore now. It does the whole compilation, sequentially. Before, this member did 2 things: - compile everything sequentially if we didn't enable parallel compilation - record the sizes of whatever was compiled, regardless how that happened. It now has one responsibility: compile sequentially. We record sizes separately. https://codereview.chromium.org/2008043006/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:605: void WasmModuleInstance::ProcessFunctionsPostLinking() { On 2016/05/26 15:12:25, titzer wrote: > Please name this function for what it does. SetDeoptimizationData(), e.g. Agreed.
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/2008043006/diff/40001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2008043006/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:605: void WasmModuleInstance::ProcessFunctionsPostLinking() { On 2016/05/26 15:53:18, Mircea Trofin wrote: > On 2016/05/26 15:12:25, titzer wrote: > > Please name this function for what it does. SetDeoptimizationData(), e.g. > > Agreed. Actually... Perhaps SetStackTracingHelpers may be better? Because that's what we're trying to achieve (if I remember correctly). The fact that we use deopt for that is the implementation detail of what we're trying to achieve.
titzer@chromium.org changed reviewers: + ahaas@chromium.org
+ahaas https://codereview.chromium.org/2008043006/diff/100001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (left): https://codereview.chromium.org/2008043006/diff/100001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:755: // 5) The main thread finishes the compilation. Looks like this step somehow go left out of the parallel compilation?
https://codereview.chromium.org/2008043006/diff/100001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (left): https://codereview.chromium.org/2008043006/diff/100001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:755: // 5) The main thread finishes the compilation. On 2016/05/27 16:03:28, titzer wrote: > Looks like this step somehow go left out of the parallel compilation? Not quite. It was doing 2 things: - compile sequentially if we didn't do parallel compilation. see line 580 on the "before" side - load up the linker and code_table, record sizes. The first part is now "just done" by sharing code vectors between instance and linker. The latter is now done for either compilations (parallel vs sequential), see line 780.
https://codereview.chromium.org/2008043006/diff/100001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2008043006/diff/100001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:127: DCHECK(index < function_code().size()); Could you add a DCHECK(function_code()[index].is_null()) here to make sure that the placeholder does not overwrite a real code object? https://codereview.chromium.org/2008043006/diff/100001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:566: results[j] = unit->FinishCompilation(); I think this will be a data race with the GetFunctionCode in the WasmLinker.
Patchset #6 (id:120001) has been deleted
Patchset #7 (id:160001) has been deleted
Patchset #7 (id:180001) has been deleted
Patchset #6 (id:140001) has been deleted
https://codereview.chromium.org/2008043006/diff/100001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2008043006/diff/100001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:127: DCHECK(index < function_code().size()); On 2016/05/30 14:02:32, ahaas wrote: > Could you add a DCHECK(function_code()[index].is_null()) here to make sure that > the placeholder does not overwrite a real code object? Done. https://codereview.chromium.org/2008043006/diff/100001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:566: results[j] = unit->FinishCompilation(); On 2016/05/30 14:02:32, ahaas wrote: > I think this will be a data race with the GetFunctionCode in the WasmLinker. Good catch, thanks! Replaced WasmLinker::GetFunctionCode with a WasmLinker::GetPlaceholderCode. After this CL, I want to refresh 1962643004, make sure it works with the serializer, and get it in. The referenced CL does away with placeholder code altogether.
lgtm
The CQ bit was checked by mtrofin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2008043006/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2008043006/210001
Message was sent while issue was closed.
Description was changed from ========== [wasm] separate snapshot-able stages This CLprepares the terrain for serialization/deserialization. It sets up the instantiation stages such that we have a point wereh we can split off obtaining the code from a snapshot, or snapshot. That point is after we compile and produce the code table, but before we attach the deoptimization info we use for stack tracing. Opportunistically, performed more cleanup to improve maintainability: - clarified sequential vs parallel compilation stages. FinishCompilation was somewhat ambiguous in that it performed a few responsibilities: compiling functions in the sequential case, and then populating the linker and code tables. - removed the "results" set, which is unnecessary. The linker simply shares the function_code vector, and so do the compilation stages. - populate the code table fixed array separately from compilation. This falls out of the decisions above. BUG= ========== to ========== [wasm] separate snapshot-able stages This CLprepares the terrain for serialization/deserialization. It sets up the instantiation stages such that we have a point wereh we can split off obtaining the code from a snapshot, or snapshot. That point is after we compile and produce the code table, but before we attach the deoptimization info we use for stack tracing. Opportunistically, performed more cleanup to improve maintainability: - clarified sequential vs parallel compilation stages. FinishCompilation was somewhat ambiguous in that it performed a few responsibilities: compiling functions in the sequential case, and then populating the linker and code tables. - removed the "results" set, which is unnecessary. The linker simply shares the function_code vector, and so do the compilation stages. - populate the code table fixed array separately from compilation. This falls out of the decisions above. BUG= ==========
Message was sent while issue was closed.
Committed patchset #7 (id:210001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] separate snapshot-able stages This CLprepares the terrain for serialization/deserialization. It sets up the instantiation stages such that we have a point wereh we can split off obtaining the code from a snapshot, or snapshot. That point is after we compile and produce the code table, but before we attach the deoptimization info we use for stack tracing. Opportunistically, performed more cleanup to improve maintainability: - clarified sequential vs parallel compilation stages. FinishCompilation was somewhat ambiguous in that it performed a few responsibilities: compiling functions in the sequential case, and then populating the linker and code tables. - removed the "results" set, which is unnecessary. The linker simply shares the function_code vector, and so do the compilation stages. - populate the code table fixed array separately from compilation. This falls out of the decisions above. BUG= ========== to ========== [wasm] separate snapshot-able stages This CLprepares the terrain for serialization/deserialization. It sets up the instantiation stages such that we have a point wereh we can split off obtaining the code from a snapshot, or snapshot. That point is after we compile and produce the code table, but before we attach the deoptimization info we use for stack tracing. Opportunistically, performed more cleanup to improve maintainability: - clarified sequential vs parallel compilation stages. FinishCompilation was somewhat ambiguous in that it performed a few responsibilities: compiling functions in the sequential case, and then populating the linker and code tables. - removed the "results" set, which is unnecessary. The linker simply shares the function_code vector, and so do the compilation stages. - populate the code table fixed array separately from compilation. This falls out of the decisions above. BUG= Committed: https://crrev.com/3d25ad4df19a75010b120967d9ef3d589979e25c Cr-Commit-Position: refs/heads/master@{#36618} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/3d25ad4df19a75010b120967d9ef3d589979e25c Cr-Commit-Position: refs/heads/master@{#36618} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
