Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2707873002:
Collect type profile for DevTools. (Closed)
DescriptionCollect type profile for DevTools
Collect type information for JavaScript variables and display it
in Chrome DevTools.
Design Doc: https://docs.google.com/a/google.com/document/d/1O1uepXZXBI6IwiawTrYC3ohhiNgzkyTdjn3R8ysbYgk/edit?usp=sharing
When debugging JavaScript, it’s helpful to know the type of
a variable, parameter, and return values. JavaScript is
dynamically typed, and for complex
source code it’s often hard to infer types. With type profiling, we
can provide type information to JavaScript developers.
This CL is a proof of concept. It collects type profile for
assignments and simply prints the types to stdout.
The output looks something like this:
#my_var1
#Object
#number
#string
#number
#undefined
#string
#Object
#Object
We use an extra slot in the feedback vector of assignments to
carry the list of types for that assignment. The extra slot is
only added when the flag --type-profile is given.
Missing work:
* Collect data for parameters and return values (currently only assignments).
* Remove duplicates from the list of collected types and use a common base class.
* Add line numbers or source position instead of the variable name.
For now, has a test that compares the stdout of --type-profile in test/message. We
will remove this test when --type-profile is fully integrated in
the debugger protocol. Adding
the test in test/inspector does not work, because the inspector
test itself consists of JavaScript code that would convolute the
output and be non-deterministic under stress.
BUG=v8:5935
Review-Url: https://codereview.chromium.org/2707873002
Cr-Commit-Position: refs/heads/master@{#43866}
Committed: https://chromium.googlesource.com/v8/v8/+/947a0437668ce21392358634494543a664c6b123
Patch Set 1 #Patch Set 2 : Always turn on the flag for now #Patch Set 3 : Delete dummy nexus call. #Patch Set 4 : Delete printf. #Patch Set 5 : Rebaseline and fix vector cctest. #Patch Set 6 : Fix vector cctest. #Patch Set 7 : Print all types seen so far for each assignment. #Patch Set 8 : Rebaseline. #Patch Set 9 : Add megamorphic handling. #Patch Set 10 : Minor fixes. #Patch Set 11 : Fix test. #Patch Set 12 : Use constructor name if available. #
Total comments: 8
Patch Set 13 : Set slot size to 1. #Patch Set 14 : Simplify Type profile feedback. #Patch Set 15 : Simplify StateFromFeedback(). #Patch Set 16 : Emit bytecode only if flag is set. #Patch Set 17 : Fix unittest #Patch Set 18 : Only make new register if flag is on. #Patch Set 19 : Fix nullptr exception. #Patch Set 20 : Small cleanups. #Patch Set 21 : Fix feedback vector test back to its original checks. #Patch Set 22 : Fix when we use type profile. #Patch Set 23 : Rebase. #Patch Set 24 : Rebase. #Patch Set 25 : Cleanup variable names in test. #Patch Set 26 : Add inspector test. #Patch Set 27 : Simplify AssignFeedbackSlot(). #Patch Set 28 : Clean up test. #Patch Set 29 : [ast] Use BitField for boolean flag. #Patch Set 30 : Add documentation and sprinkle consts around. #
Total comments: 15
Patch Set 31 : Address Yang's comments. #Patch Set 32 : Add more exhaustive test case. #Patch Set 33 : Check FeedbackMetadata to decide if we can collect type profile. #Patch Set 34 : Fix DCHECK in FeedbackParameter JS Operator. #Patch Set 35 : Delete unused constants. #Patch Set 36 : Fix typo. #
Total comments: 2
Patch Set 37 : Use FeedbackMetadataIterator. #Patch Set 38 : Delete unused var #
Total comments: 11
Patch Set 39 : Delete Opcode. #Patch Set 40 : Use ToCString() instead of debug Print. #Patch Set 41 : Collect type profile only for user JS. #Patch Set 42 : Move tests to message instead of inspector tests. #Patch Set 43 : Move tests to message instead of inspector tests. #Patch Set 44 : Collect type profile only for user JS. #Patch Set 45 : Throw in test to make message test work. Yikes. #Patch Set 46 : Explain why throw is needed in message test. #Dependent Patchsets: Messages
Total messages: 213 (174 generated)
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...) v8_win_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/21396) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/22991) v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== WIP: bytecode and runtime time function for type profiling. Generate bytecode to call runtime function for type profile. BUG=v8:5935 ========== to ========== WIP: Collect type profile. Collect type profile for variables. In the future, we want to show this information in Chrome Developer Tools. This allows the developer to better understand what types (e.g., string, undef, object, custom classes) a variable, parameter, or return value has been set to. This implementation of type profile requires extra bytecode and slots in the feedback vector and can thus not be turned on by default but only in debug mode when the type profile should be collected. As proof of concept, this implementation simply prints encountered types to stdout. We piggyback on feedback vectors to store type profile, i.e., an array of names of all types so far. WIP: * Collect data for assignments, needs to include parameters and return values. * Reserving slot when flag is turned off * Simply collect all types instead of a unique list * constructor.name is not used * line number or source position is missing. * tests are missing. * reserving 2 feedback slots, but extra feedback is not needed. BUG=v8:5935 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from
==========
WIP: Collect type profile.
Collect type profile for variables. In the future,
we want to show this information in Chrome Developer Tools.
This allows the developer
to better understand what types (e.g., string, undef, object, custom
classes) a variable, parameter, or return value has been set to.
This implementation of type profile requires extra bytecode and slots
in the feedback vector and can thus not be turned on by
default but only in debug mode when the type profile should
be collected.
As proof of concept, this implementation simply prints
encountered types to stdout.
We piggyback on feedback vectors to store type profile, i.e.,
an array of names of all types so far.
WIP:
* Collect data for assignments, needs to include parameters and return values.
* Reserving slot when flag is turned off
* Simply collect all types instead of a unique list
* constructor.name is not used
* line number or source position is missing.
* tests are missing.
* reserving 2 feedback slots, but extra feedback is not needed.
BUG=v8:5935
==========
to
==========
WIP: Collect type profile.
Collect type profile for variables. In the future,
we want to show this information in Chrome Developer Tools.
This allows the developer
to better understand what types (e.g., string, undef, object, custom
classes) a variable, parameter, or return value has been set to.
This implementation of type profile requires extra bytecode and slots
in the feedback vector and can thus not be turned on by
default but only in debug mode when the type profile should
be collected, i.e., with --type-profile.
As proof of concept, this implementation simply prints
encountered types to stdout.
We piggyback on feedback vectors to store type profile, i.e.,
an array of names of all types so far.
The output looks something like this:
#my_var
#object
#my_var
#object
#my_var
#object
#my_var
#object
#my_var
0xd750b294919: [FixedArray]
- map = 0x20a9c6f82309 <Map(FAST_HOLEY_ELEMENTS)>
- length: 5
0: 2
1: 0x11d5c6582271 <String[6]: object>
2: 0x11d5c65834a9 <String[6]: number>
3-4: 0x11d5c6582311 <undefined>
#my_var
0xd750b294919: [FixedArray]
- map = 0x20a9c6f82309 <Map(FAST_HOLEY_ELEMENTS)>
- length: 5
0: 3
1: 0x11d5c6582271 <String[6]: object>
2: 0x11d5c65834a9 <String[6]: number>
3: 0x11d5c6583989 <String[6]: string>
4: 0x11d5c6582311 <undefined>
#my_var
0xd750b294919: [FixedArray]
- map = 0x20a9c6f82309 <Map(FAST_HOLEY_ELEMENTS)>
- length: 5
0: 4
1: 0x11d5c6582271 <String[6]: object>
2: 0x11d5c65834a9 <String[6]: number>
3: 0x11d5c6583989 <String[6]: string>
4: 0x11d5c65834a9 <String[6]: number>
#my_var
Too many types to list them.
#my_var
Too many types to list them.
#my_var
Too many types to list them.
WIP:
* Collect data for assignments, needs to include parameters and return values.
* Reserving slot when flag is turned off
* Simply collect all types instead of a unique list
* constructor.name is not used
* line number or source position is missing.
* tests are missing.
* reserving 2 feedback slots, but extra feedback is not needed.
BUG=v8:5935
==========
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
franzih@chromium.org changed reviewers: + yangguo@chromium.org
Hi Yang, Could you have a look if I'm on the right track please? Thanks, Franzi
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looking good. Some comments. https://codereview.chromium.org/2707873002/diff/190001/src/feedback-vector-inl.h File src/feedback-vector-inl.h (right): https://codereview.chromium.org/2707873002/diff/190001/src/feedback-vector-in... src/feedback-vector-inl.h:71: return 2; Why do we need two slots for this? https://codereview.chromium.org/2707873002/diff/190001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/2707873002/diff/190001/src/flag-definitions.h... src/flag-definitions.h:297: DEFINE_BOOL(trace_strong_rooted_literals, false, "trace literal rooting") where does this come from? https://codereview.chromium.org/2707873002/diff/190001/src/interpreter/byteco... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2707873002/diff/190001/src/interpreter/byteco... src/interpreter/bytecode-generator.cc:2222: if (FLAG_type_profile) { The issue with this flag and snapshot is: if the function is part of the snapshot, it may already have a feedback metadata created with this flag off. When compiling the function here, we assume the feedback metadata to have been created with this flag on. Either we clear the feedback metadata when we serialize, along with the code, or we figure out from the feedback metadata, if it exists, whether we have slots reserved for type profile, instead of relying on the flag. I'll test whether clearing the feedback metadata during serialization can be done. We should not collect type profile for functions for which SharedFunctionInfo::IsSubjectToDebugging returns false though, which every function in the default startup snapshot does. https://codereview.chromium.org/2707873002/diff/190001/src/interpreter/byteco... src/interpreter/bytecode-generator.cc:2368: builder()->CollectTypeProfile(lhs_name, value, We would at some point need a mapping from feedback slot to source position and name, so that when we harvest the data collected in the feedback slots, we can tell where to attribute it to. In that case, all we need as arguments here are the value, and the vector slot pair. We can then use the mapping to get the name and source position. That's a bit less intrusive than getting names. https://codereview.chromium.org/2707873002/diff/190001/src/runtime/runtime-ob... File src/runtime/runtime-object.cc (right): https://codereview.chromium.org/2707873002/diff/190001/src/runtime/runtime-ob... src/runtime/runtime-object.cc:694: CollectTypeProfileICNexus nexus(vector, vector->ToSlot(index)); Do we need to fit this into the IC framework? We don't really care about whether it's monomorphic/polymorphic/megamorphic. And I don't think setting an arbitrary megamorphic limit makes sense at this point. Let's just add the types onto the ArrayList without making this look like an IC.
I took a brief look, and it seems removing all feedback metadata and vectors during serialization is not feasible. We have two ways to go forward here: 1) Encode into the shared function info (compiler hints) or feedback metadata whether it reserved slots for type profiling, when it's created. If during AST numbering, we have no shared function info on the parse info, we reserve the slots (also depending on FLAG_type_profile). Otherwise we check what we encoded on the shared function info. 2) Not use feedback vector at all, but store the information on DebugInfo. During AST numbering, we would assign an index to every variable. We then create a FixedArray for the type profile on the DebugInfo of the SharedFunctionInfo, with two element for every variable. The first element is the observed type (or an ArrayList for multiple types), the second element is the source position of the variable (this solves the mapping to source position that we need later). When executing the bytecode, we call into the runtime with the feedback vector, the variable index, and the value. We use the feedback vector to get to the shared function info. From the shared function info we get to the debug info, and from the debug info the the type profile FixedArray. That way, we do not run into inconsistent feedback metadata. I can live with either solution. The latter is a bit more work to implement though, from what you already have.
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from
==========
WIP: Collect type profile.
Collect type profile for variables. In the future,
we want to show this information in Chrome Developer Tools.
This allows the developer
to better understand what types (e.g., string, undef, object, custom
classes) a variable, parameter, or return value has been set to.
This implementation of type profile requires extra bytecode and slots
in the feedback vector and can thus not be turned on by
default but only in debug mode when the type profile should
be collected, i.e., with --type-profile.
As proof of concept, this implementation simply prints
encountered types to stdout.
We piggyback on feedback vectors to store type profile, i.e.,
an array of names of all types so far.
The output looks something like this:
#my_var
#object
#my_var
#object
#my_var
#object
#my_var
#object
#my_var
0xd750b294919: [FixedArray]
- map = 0x20a9c6f82309 <Map(FAST_HOLEY_ELEMENTS)>
- length: 5
0: 2
1: 0x11d5c6582271 <String[6]: object>
2: 0x11d5c65834a9 <String[6]: number>
3-4: 0x11d5c6582311 <undefined>
#my_var
0xd750b294919: [FixedArray]
- map = 0x20a9c6f82309 <Map(FAST_HOLEY_ELEMENTS)>
- length: 5
0: 3
1: 0x11d5c6582271 <String[6]: object>
2: 0x11d5c65834a9 <String[6]: number>
3: 0x11d5c6583989 <String[6]: string>
4: 0x11d5c6582311 <undefined>
#my_var
0xd750b294919: [FixedArray]
- map = 0x20a9c6f82309 <Map(FAST_HOLEY_ELEMENTS)>
- length: 5
0: 4
1: 0x11d5c6582271 <String[6]: object>
2: 0x11d5c65834a9 <String[6]: number>
3: 0x11d5c6583989 <String[6]: string>
4: 0x11d5c65834a9 <String[6]: number>
#my_var
Too many types to list them.
#my_var
Too many types to list them.
#my_var
Too many types to list them.
WIP:
* Collect data for assignments, needs to include parameters and return values.
* Reserving slot when flag is turned off
* Simply collect all types instead of a unique list
* constructor.name is not used
* line number or source position is missing.
* tests are missing.
* reserving 2 feedback slots, but extra feedback is not needed.
BUG=v8:5935
==========
to
==========
WIP: Collect type profile.
Collect type profile for variables. In the future,
we want to show this information in Chrome Developer Tools.
This allows the developer
to better understand what types (e.g., string, undef, object, custom
classes) a variable, parameter, or return value has been set to.
This implementation of type profile requires extra bytecode and slots
in the feedback vector and can thus not be turned on by
default but only in debug mode when the type profile should
be collected, i.e., with --type-profile.
As proof of concept, this implementation simply prints
encountered types to stdout.
We piggyback on feedback vectors to store type profile, i.e.,
an array of names of all types so far.
The output looks something like this:
#my_var
0xd750b294919: [FixedArray]
- map = 0x20a9c6f82309 <Map(FAST_HOLEY_ELEMENTS)>
- length: 5
0: 4
1: 0x11d5c6582271 <String[6]: object>
2: 0x11d5c65834a9 <String[6]: number>
3: 0x11d5c6583989 <String[6]: string>
4: 0x11d5c65834a9 <String[6]: number>
Missing work:
* Collect data for parameters and return values (currently only assignments).
* Do not reserve slot when the flag is turned off
* Remove duplicates from collected types, maybe use a common base class.
* Add line number or source position.
* Tests are missing.
BUG=v8:5935
==========
Description was changed from
==========
WIP: Collect type profile.
Collect type profile for variables. In the future,
we want to show this information in Chrome Developer Tools.
This allows the developer
to better understand what types (e.g., string, undef, object, custom
classes) a variable, parameter, or return value has been set to.
This implementation of type profile requires extra bytecode and slots
in the feedback vector and can thus not be turned on by
default but only in debug mode when the type profile should
be collected, i.e., with --type-profile.
As proof of concept, this implementation simply prints
encountered types to stdout.
We piggyback on feedback vectors to store type profile, i.e.,
an array of names of all types so far.
The output looks something like this:
#my_var
0xd750b294919: [FixedArray]
- map = 0x20a9c6f82309 <Map(FAST_HOLEY_ELEMENTS)>
- length: 5
0: 4
1: 0x11d5c6582271 <String[6]: object>
2: 0x11d5c65834a9 <String[6]: number>
3: 0x11d5c6583989 <String[6]: string>
4: 0x11d5c65834a9 <String[6]: number>
Missing work:
* Collect data for parameters and return values (currently only assignments).
* Do not reserve slot when the flag is turned off
* Remove duplicates from collected types, maybe use a common base class.
* Add line number or source position.
* Tests are missing.
BUG=v8:5935
==========
to
==========
WIP: Collect type profile.
Collect type profile for variables. In the future,
we want to show this information in Chrome Developer Tools.
This allows the developer
to better understand what types (e.g., string, undef, object, custom
classes) a variable, parameter, or return value has been set to.
This implementation of type profile requires extra bytecode and slots
in the feedback vector and can thus not be turned on by
default but only in debug mode when the type profile should
be collected, i.e., with --type-profile.
As proof of concept, this implementation simply prints
encountered types to stdout.
We piggyback on feedback vectors to store type profile, i.e.,
an array of names of all types so far.
The output looks something like this:
#my_var
0xd750b294919: [FixedArray]
- map = 0x20a9c6f82309 <Map(FAST_HOLEY_ELEMENTS)>
- length: 5
0: 4
1: 0x11d5c6582271 <String[6]: object>
2: 0x11d5c65834a9 <String[6]: number>
3: 0x11d5c6583989 <String[6]: string>
4: 0x11d5c65834a9 <String[6]: number>
Missing work:
* Collect data for parameters and return values (currently only assignments).
* Do not reserve slot when the flag is turned off
* Remove duplicates from collected types, maybe use a common base class.
* Add line number or source position.
* Tests are missing.
* Do not pass the variable name to the runtime, use accumulator for value.
BUG=v8:5935
==========
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2707873002/diff/190001/src/feedback-vector-inl.h File src/feedback-vector-inl.h (right): https://codereview.chromium.org/2707873002/diff/190001/src/feedback-vector-in... src/feedback-vector-inl.h:71: return 2; On 2017/02/22 10:39:28, Yang wrote: > Why do we need two slots for this? Changed it to 1. https://codereview.chromium.org/2707873002/diff/190001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/2707873002/diff/190001/src/flag-definitions.h... src/flag-definitions.h:297: DEFINE_BOOL(trace_strong_rooted_literals, false, "trace literal rooting") On 2017/02/22 10:39:28, Yang wrote: > where does this come from? Deleted. I probably messed up during rebase. https://codereview.chromium.org/2707873002/diff/190001/src/runtime/runtime-ob... File src/runtime/runtime-object.cc (right): https://codereview.chromium.org/2707873002/diff/190001/src/runtime/runtime-ob... src/runtime/runtime-object.cc:694: CollectTypeProfileICNexus nexus(vector, vector->ToSlot(index)); On 2017/02/22 10:39:28, Yang wrote: > Do we need to fit this into the IC framework? We don't really care about whether > it's monomorphic/polymorphic/megamorphic. And I don't think setting an arbitrary > megamorphic limit makes sense at this point. > > Let's just add the types onto the ArrayList without making this look like an IC. Yes, IC framework makes no sense for types. Simplified it.
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
franzih@chromium.org changed reviewers: + mvstanton@chromium.org
franzih@chromium.org changed reviewers: + marja@chromium.org, rmcilroy@chromium.org
franzih@chromium.org changed reviewers: + jarin@chromium.org
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
WIP: Collect type profile.
Collect type profile for variables. In the future,
we want to show this information in Chrome Developer Tools.
This allows the developer
to better understand what types (e.g., string, undef, object, custom
classes) a variable, parameter, or return value has been set to.
This implementation of type profile requires extra bytecode and slots
in the feedback vector and can thus not be turned on by
default but only in debug mode when the type profile should
be collected, i.e., with --type-profile.
As proof of concept, this implementation simply prints
encountered types to stdout.
We piggyback on feedback vectors to store type profile, i.e.,
an array of names of all types so far.
The output looks something like this:
#my_var
0xd750b294919: [FixedArray]
- map = 0x20a9c6f82309 <Map(FAST_HOLEY_ELEMENTS)>
- length: 5
0: 4
1: 0x11d5c6582271 <String[6]: object>
2: 0x11d5c65834a9 <String[6]: number>
3: 0x11d5c6583989 <String[6]: string>
4: 0x11d5c65834a9 <String[6]: number>
Missing work:
* Collect data for parameters and return values (currently only assignments).
* Do not reserve slot when the flag is turned off
* Remove duplicates from collected types, maybe use a common base class.
* Add line number or source position.
* Tests are missing.
* Do not pass the variable name to the runtime, use accumulator for value.
BUG=v8:5935
==========
to
==========
WIP: Collect type profile.
Collect type profile for variables. In the future,
we want to show this information in Chrome Developer Tools.
This allows the developer
to better understand what types (e.g., string, undef, object, custom
classes) a variable, parameter, or return value has been set to.
This implementation of type profile requires extra bytecode and slots
in the feedback vector and can thus not be turned on by
default but only in debug mode when the type profile should
be collected, i.e., with --type-profile.
As proof of concept, this implementation simply prints
encountered types to stdout.
We piggyback on feedback vectors to store type profile, i.e.,
an array of names of all types so far.
The output looks something like this:
#my_var
0xd750b294919: [FixedArray]
- map = 0x20a9c6f82309 <Map(FAST_HOLEY_ELEMENTS)>
- length: 5
0: 4
1: 0x11d5c6582271 <String[6]: object>
2: 0x11d5c65834a9 <String[6]: number>
3: 0x11d5c6583989 <String[6]: string>
4: 0x11d5c65834a9 <String[6]: number>
Missing work:
* Collect data for parameters and return values (currently only assignments).
* Remove duplicates from list of collected types and use a common base class.
* Add line number or source position.
BUG=v8:5935
==========
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from
==========
WIP: Collect type profile.
Collect type profile for variables. In the future,
we want to show this information in Chrome Developer Tools.
This allows the developer
to better understand what types (e.g., string, undef, object, custom
classes) a variable, parameter, or return value has been set to.
This implementation of type profile requires extra bytecode and slots
in the feedback vector and can thus not be turned on by
default but only in debug mode when the type profile should
be collected, i.e., with --type-profile.
As proof of concept, this implementation simply prints
encountered types to stdout.
We piggyback on feedback vectors to store type profile, i.e.,
an array of names of all types so far.
The output looks something like this:
#my_var
0xd750b294919: [FixedArray]
- map = 0x20a9c6f82309 <Map(FAST_HOLEY_ELEMENTS)>
- length: 5
0: 4
1: 0x11d5c6582271 <String[6]: object>
2: 0x11d5c65834a9 <String[6]: number>
3: 0x11d5c6583989 <String[6]: string>
4: 0x11d5c65834a9 <String[6]: number>
Missing work:
* Collect data for parameters and return values (currently only assignments).
* Remove duplicates from list of collected types and use a common base class.
* Add line number or source position.
BUG=v8:5935
==========
to
==========
WIP: Collect type profile.
Collect type profile for variables. In the future,
we want to show this information in Chrome Developer Tools.
This allows the developer
to better understand what types (e.g., string, undef, object, custom
classes) a variable, parameter, or return value has been set to.
This implementation of type profile requires extra bytecode and slots
in the feedback vector and can thus not be turned on by
default but only in debug mode when the type profile should
be collected, i.e., with --type-profile.
As proof of concept, this implementation simply prints
encountered types to stdout.
We piggyback on feedback vectors to store type profile, i.e.,
an array of names of all types so far.
The output looks something like this:
#my_var1
#Object
#number
#string
#number
#undefined
#string
#Object
#Object
Missing work:
* Collect data for parameters and return values (currently only assignments).
* Remove duplicates from list of collected types and use a common base class.
* Add line number or source position.
BUG=v8:5935
==========
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== WIP: Collect type profile. Collect type profile for variables. In the future, we want to show this information in Chrome Developer Tools. This allows the developer to better understand what types (e.g., string, undef, object, custom classes) a variable, parameter, or return value has been set to. This implementation of type profile requires extra bytecode and slots in the feedback vector and can thus not be turned on by default but only in debug mode when the type profile should be collected, i.e., with --type-profile. As proof of concept, this implementation simply prints encountered types to stdout. We piggyback on feedback vectors to store type profile, i.e., an array of names of all types so far. The output looks something like this: #my_var1 #Object #number #string #number #undefined #string #Object #Object Missing work: * Collect data for parameters and return values (currently only assignments). * Remove duplicates from list of collected types and use a common base class. * Add line number or source position. BUG=v8:5935 ========== to ========== Collect type profile for DevTools Collect type information for JavaScript variables and display it in Chrome DevTools. Design Doc: https://docs.google.com/a/google.com/document/d/1O1uepXZXBI6IwiawTrYC3ohhiNgz... When debugging JavaScript, it’s helpful to know the type of a variable, parameter, and return values. JavaScript is dynamically typed, and for complex source code it’s often hard to infer types. With type profiling, we can provide type information to JavaScript developers. This CL is a proof of concept. It collects type profile for assignments and simply prints the types to stdout. The output looks something like this: #my_var1 #Object #number #string #number #undefined #string #Object #Object We use an extra slot in the feedback vector of assignments to carry the list of types for that assignment. The extra slot is only added when the flag --type-profile is given. Missing work: * Collect data for parameters and return values (currently only assignments). * Remove duplicates from the list of collected types and use a common base class. * Add line numbers or source position instead of the variable name. BUG=v8:5935 ==========
franzih@chromium.org changed reviewers: - jarin@chromium.org, marja@chromium.org, rmcilroy@chromium.org
Hi Michael, Could you have a look at my Type Profile CL? Yang said you have some suggestions about using the feedback vector differently? Yang, I addressed your comments. Could you have another look? Thanks, Franzi
Hi Michael, Could you have a look at my Type Profile CL? Yang said you have some suggestions about using the feedback vector differently? Yang, I addressed your comments. Could you have another look? Thanks, Franzi
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Very nice. LGTM. Michael, please also take a look. My comments are supposed to kick off discussions on the next steps. https://codereview.chromium.org/2707873002/diff/550001/src/ast/ast.h File src/ast/ast.h (right): https://codereview.chromium.org/2707873002/diff/550001/src/ast/ast.h#newcode2421 src/ast/ast.h:2421: FeedbackSlot CollectTypeProfileSlot() const { CollectTypeProfileSlot sounds like an action. Can we rename this to TypeProfileSlot? https://codereview.chromium.org/2707873002/diff/550001/src/ast/ast.h#newcode2427 src/ast/ast.h:2427: return CollectTypeProfileSlotField::decode(bit_field_); Do we really need this bit in the bit field? Can we just check whether the FeedbackSlot is invalid? https://codereview.chromium.org/2707873002/diff/550001/src/compiler.cc File src/compiler.cc (right): https://codereview.chromium.org/2707873002/diff/550001/src/compiler.cc#newcod... src/compiler.cc:329: if (!info->shared_info()->computed_collects_type_profile()) { Michael, here's where I'd like you to take a closer look. What we want to achieve here is: - We can freely make the decision whether to collect type profile at the first compile. This decision is made based on a command line flag. - After the first compile, we need to persist the decision somehow, so that any subsequent compiles do not conflict with existing feedback metadata. - We can run into the situation of compiling with existing feedback metadata if we compile a function from the snapshot, since snapshotting discards the code. Currently this decision is persisted on the shared function info. So we are assuming that this is the correct place to keep this decision in sync with the feedback metadata. But I wonder whether we persist this decision even closer to the feedback metadata. For example, if we could figure out the previous decision from looking at feedback metadata itself, that would be a lot nicer. https://codereview.chromium.org/2707873002/diff/550001/src/compiler.cc#newcod... src/compiler.cc:504: parse_info->shared_info()->collects_type_profile()); This expression seems correct. But can we make this a bit more readable? Something like if (parse_info->shared_info().is_null() || !parse_info->shared_info()->computed_collects_type_profile())) { collect_type_profile = FLAG_type_profile } else { collect_type_profile = parse_info->shared_info()->collects_type_profile(); } https://codereview.chromium.org/2707873002/diff/550001/src/feedback-vector.cc File src/feedback-vector.cc (right): https://codereview.chromium.org/2707873002/diff/550001/src/feedback-vector.cc... src/feedback-vector.cc:1053: SetFeedback(*ArrayList::Add(types, type)); We could use a StringSet here. https://codereview.chromium.org/2707873002/diff/550001/src/interpreter/byteco... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2707873002/diff/550001/src/interpreter/byteco... src/interpreter/bytecode-generator.cc:2217: Register lhs_name; Do we have a plan to use a better way to identify variables than names? Variable names could be ambiguous due to shadowing, and we would need a mapping from feedback slot to name and source position that we can use when we harvest all type profile information at once. I wonder whether we can store this mapping in the feedback metadata. Maybe we only need the mapping from slot to bytecode offset of the declaration, and from there we could extract the variable name and the source position from the bytecode array and source position table. https://codereview.chromium.org/2707873002/diff/550001/src/runtime/runtime-ob... File src/runtime/runtime-object.cc (right): https://codereview.chromium.org/2707873002/diff/550001/src/runtime/runtime-ob... src/runtime/runtime-object.cc:715: printf("\n"); Let's use PrintF (uppercase). Otherwise android tests would fail, iirc.
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
This should help for now, and I'll look at the rest when you plug that new feedback metadata function in, thx! https://codereview.chromium.org/2707873002/diff/550001/src/compiler.cc File src/compiler.cc (right): https://codereview.chromium.org/2707873002/diff/550001/src/compiler.cc#newcod... src/compiler.cc:329: if (!info->shared_info()->computed_collects_type_profile()) { On 2017/03/09 10:28:27, Yang wrote: > Michael, here's where I'd like you to take a closer look. What we want to > achieve here is: > - We can freely make the decision whether to collect type profile at the first > compile. This decision is made based on a command line flag. > - After the first compile, we need to persist the decision somehow, so that any > subsequent compiles do not conflict with existing feedback metadata. > - We can run into the situation of compiling with existing feedback metadata if > we compile a function from the snapshot, since snapshotting discards the code. > > Currently this decision is persisted on the shared function info. So we are > assuming that this is the correct place to keep this decision in sync with the > feedback metadata. > > But I wonder whether we persist this decision even closer to the feedback > metadata. For example, if we could figure out the previous decision from looking > at feedback metadata itself, that would be a lot nicer. Yep, it makes sense to avoid these flags and just iterate the feedback metadata looking for your special slot. If you find one, then the metadata was created at a moment when you wanted type profiling, so now you know that.
Yang, I addressed your comments. Left to do: Do not store collects_type_profile on the shared function info. Instead check the FeedbackMetadata whether it has any slots of kind TypeProfile. https://codereview.chromium.org/2707873002/diff/550001/src/ast/ast.h File src/ast/ast.h (right): https://codereview.chromium.org/2707873002/diff/550001/src/ast/ast.h#newcode2421 src/ast/ast.h:2421: FeedbackSlot CollectTypeProfileSlot() const { On 2017/03/09 10:28:27, Yang wrote: > CollectTypeProfileSlot sounds like an action. Can we rename this to > TypeProfileSlot? Done. https://codereview.chromium.org/2707873002/diff/550001/src/ast/ast.h#newcode2427 src/ast/ast.h:2427: return CollectTypeProfileSlotField::decode(bit_field_); On 2017/03/09 10:28:27, Yang wrote: > Do we really need this bit in the bit field? Can we just check whether the > FeedbackSlot is invalid? Done. https://codereview.chromium.org/2707873002/diff/550001/src/compiler.cc File src/compiler.cc (right): https://codereview.chromium.org/2707873002/diff/550001/src/compiler.cc#newcod... src/compiler.cc:504: parse_info->shared_info()->collects_type_profile()); On 2017/03/09 10:28:27, Yang wrote: > This expression seems correct. But can we make this a bit more readable? > Something like > > if (parse_info->shared_info().is_null() || > !parse_info->shared_info()->computed_collects_type_profile())) { > collect_type_profile = FLAG_type_profile > } else { > collect_type_profile = parse_info->shared_info()->collects_type_profile(); > } Thanks for the suggestions. Done. https://codereview.chromium.org/2707873002/diff/550001/src/feedback-vector.cc File src/feedback-vector.cc (right): https://codereview.chromium.org/2707873002/diff/550001/src/feedback-vector.cc... src/feedback-vector.cc:1053: SetFeedback(*ArrayList::Add(types, type)); On 2017/03/09 10:28:27, Yang wrote: > We could use a StringSet here. Ok. Didn't know we have those. If it's OK, I'll do that in a separate commit because we need to sort/simplify the list anyways. https://codereview.chromium.org/2707873002/diff/550001/src/interpreter/byteco... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2707873002/diff/550001/src/interpreter/byteco... src/interpreter/bytecode-generator.cc:2217: Register lhs_name; On 2017/03/09 10:28:27, Yang wrote: > Do we have a plan to use a better way to identify variables than names? Variable > names could be ambiguous due to shadowing, and we would need a mapping from > feedback slot to name and source position that we can use when we harvest all > type profile information at once. I wonder whether we can store this mapping in > the feedback metadata. > > Maybe we only need the mapping from slot to bytecode offset of the declaration, > and from there we could extract the variable name and the source position from > the bytecode array and source position table. I'm only passing the name so that I can understand the debug output. My plan is to not use the name at all. https://codereview.chromium.org/2707873002/diff/550001/src/runtime/runtime-ob... File src/runtime/runtime-object.cc (right): https://codereview.chromium.org/2707873002/diff/550001/src/runtime/runtime-ob... src/runtime/runtime-object.cc:715: printf("\n"); On 2017/03/09 10:28:27, Yang wrote: > Let's use PrintF (uppercase). Otherwise android tests would fail, iirc. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Michael, I deleted tha flags and instead iterate over the feedback metadata. Please take another look. Thanks Franzi https://codereview.chromium.org/2707873002/diff/550001/src/compiler.cc File src/compiler.cc (right): https://codereview.chromium.org/2707873002/diff/550001/src/compiler.cc#newcod... src/compiler.cc:329: if (!info->shared_info()->computed_collects_type_profile()) { On 2017/03/10 12:12:24, mvstanton wrote: > On 2017/03/09 10:28:27, Yang wrote: > > Michael, here's where I'd like you to take a closer look. What we want to > > achieve here is: > > - We can freely make the decision whether to collect type profile at the first > > compile. This decision is made based on a command line flag. > > - After the first compile, we need to persist the decision somehow, so that > any > > subsequent compiles do not conflict with existing feedback metadata. > > - We can run into the situation of compiling with existing feedback metadata > if > > we compile a function from the snapshot, since snapshotting discards the code. > > > > Currently this decision is persisted on the shared function info. So we are > > assuming that this is the correct place to keep this decision in sync with the > > feedback metadata. > > > > But I wonder whether we persist this decision even closer to the feedback > > metadata. For example, if we could figure out the previous decision from > looking > > at feedback metadata itself, that would be a lot nicer. > > Yep, it makes sense to avoid these flags and just iterate the feedback metadata > looking for your special slot. If you find one, then the metadata was created at > a moment when you wanted type profiling, so now you know that. I deleted the constants and instead check the FeedbackMetadata.
Velly nice, LGTM, one nit. I think it'd be good for Ross to have a look at the interpreter changes. https://codereview.chromium.org/2707873002/diff/670001/src/feedback-vector.cc File src/feedback-vector.cc (right): https://codereview.chromium.org/2707873002/diff/670001/src/feedback-vector.cc... src/feedback-vector.cc:165: FeedbackSlotKind kind = GetKind(FeedbackSlot(i)); nit: There is also a feedback metadata iterator class. I think you should use that in order to avoid that tricky slot size code at the bottom of the loop. It's used elsewhere in this file.
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
franzih@chromium.org changed reviewers: + rmcilroy@chromium.org
Thanks, fixed the nit. Ross, could you have a look at the interpreter part? Thanks Franzi https://codereview.chromium.org/2707873002/diff/670001/src/feedback-vector.cc File src/feedback-vector.cc (right): https://codereview.chromium.org/2707873002/diff/670001/src/feedback-vector.cc... src/feedback-vector.cc:165: FeedbackSlotKind kind = GetKind(FeedbackSlot(i)); On 2017/03/10 14:57:23, mvstanton wrote: > nit: There is also a feedback metadata iterator class. I think you should use > that in order to avoid that tricky slot size code at the bottom of the loop. > It's used elsewhere in this file. Thanks. Fixed!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Nice, LGTM with a couple of questions. https://codereview.chromium.org/2707873002/diff/710001/src/compiler/bytecode-... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/2707873002/diff/710001/src/compiler/bytecode-... src/compiler/bytecode-graph-builder.cc:814: const Operator* op = javascript()->CollectTypeProfile(feedback); Do we want to collect type profile data in optimized code as well as unoptimized code? I don't think we update any other type feedback slots in TF code, and having this in the graph for each assignment would probably cause TF not to be able to do a bunch of optimizations. Maybe we should just treat this as a noop in TF? https://codereview.chromium.org/2707873002/diff/710001/src/interpreter/byteco... File src/interpreter/bytecode-array-builder.h (right): https://codereview.chromium.org/2707873002/diff/710001/src/interpreter/byteco... src/interpreter/bytecode-array-builder.h:142: // TODO(franzih): Do not pass the name, instead use the source position. Any not to use source position now? This would probably make the changes in VisitAssignment much simpler (since the source position could be an immediate, so no need to allocate a register and get the name differently depending on LHS. Fine with landing this as-is if you want to iterate before doing this though. https://codereview.chromium.org/2707873002/diff/710001/src/runtime/runtime-ob... File src/runtime/runtime-object.cc (right): https://codereview.chromium.org/2707873002/diff/710001/src/runtime/runtime-ob... src/runtime/runtime-object.cc:715: PrintF("\n"); Leftover PrintF debugging?
Thanks for the reviews. I'm landing as is since all the nits will be changed in the next iterations. https://codereview.chromium.org/2707873002/diff/710001/src/compiler/bytecode-... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/2707873002/diff/710001/src/compiler/bytecode-... src/compiler/bytecode-graph-builder.cc:814: const Operator* op = javascript()->CollectTypeProfile(feedback); On 2017/03/13 10:50:59, rmcilroy wrote: > Do we want to collect type profile data in optimized code as well as unoptimized > code? I don't think we update any other type feedback slots in TF code, and > having this in the graph for each assignment would probably cause TF not to be > able to do a bunch of optimizations. Maybe we should just treat this as a noop > in TF? I think we want to collect types for optimized code as well (or is nothing optimized once we're in debug mode?) https://codereview.chromium.org/2707873002/diff/710001/src/interpreter/byteco... File src/interpreter/bytecode-array-builder.h (right): https://codereview.chromium.org/2707873002/diff/710001/src/interpreter/byteco... src/interpreter/bytecode-array-builder.h:142: // TODO(franzih): Do not pass the name, instead use the source position. On 2017/03/13 10:50:59, rmcilroy wrote: > Any not to use source position now? This would probably make the changes in > VisitAssignment much simpler (since the source position could be an immediate, > so no need to allocate a register and get the name differently depending on LHS. > Fine with landing this as-is if you want to iterate before doing this though. I'll land as is to keep CL somewhat shorter. https://codereview.chromium.org/2707873002/diff/710001/src/runtime/runtime-ob... File src/runtime/runtime-object.cc (right): https://codereview.chromium.org/2707873002/diff/710001/src/runtime/runtime-ob... src/runtime/runtime-object.cc:715: PrintF("\n"); On 2017/03/13 10:50:59, rmcilroy wrote: > Leftover PrintF debugging? It's part of the output. At the moment I'm dumping everything to stdout instead of using the debugger protocol.
The CQ bit was checked by franzih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org, mvstanton@chromium.org Link to the patchset: https://codereview.chromium.org/2707873002/#ps710001 (title: "Delete unused var")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2707873002/diff/710001/src/compiler/bytecode-... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/2707873002/diff/710001/src/compiler/bytecode-... src/compiler/bytecode-graph-builder.cc:814: const Operator* op = javascript()->CollectTypeProfile(feedback); On 2017/03/13 12:01:56, Franzi wrote: > On 2017/03/13 10:50:59, rmcilroy wrote: > > Do we want to collect type profile data in optimized code as well as > unoptimized > > code? I don't think we update any other type feedback slots in TF code, and > > having this in the graph for each assignment would probably cause TF not to be > > able to do a bunch of optimizations. Maybe we should just treat this as a > noop > > in TF? > > I think we want to collect types for optimized code as well (or is nothing > optimized once we're in debug mode?) I thought generally nothing was optimized once we are in debug mode (certainly things like breakpoints and such would no longer work as expected), but I may be wrong about this - Yang any comment on this?
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/36442)
franzih@chromium.org changed reviewers: + mstarzinger@chromium.org
Hi Michi, Can I get a review for ast and compiler changes please? Thanks, Franzi
On 2017/03/13 12:05:45, rmcilroy wrote: > https://codereview.chromium.org/2707873002/diff/710001/src/compiler/bytecode-... > File src/compiler/bytecode-graph-builder.cc (right): > > https://codereview.chromium.org/2707873002/diff/710001/src/compiler/bytecode-... > src/compiler/bytecode-graph-builder.cc:814: const Operator* op = > javascript()->CollectTypeProfile(feedback); > On 2017/03/13 12:01:56, Franzi wrote: > > On 2017/03/13 10:50:59, rmcilroy wrote: > > > Do we want to collect type profile data in optimized code as well as > > unoptimized > > > code? I don't think we update any other type feedback slots in TF code, and > > > having this in the graph for each assignment would probably cause TF not to > be > > > able to do a bunch of optimizations. Maybe we should just treat this as a > > noop > > > in TF? > > > > I think we want to collect types for optimized code as well (or is nothing > > optimized once we're in debug mode?) > > I thought generally nothing was optimized once we are in debug mode (certainly > things like breakpoints and such would no longer work as expected), but I may be > wrong about this - Yang any comment on this? We generally allow optimizations. We only disable optimization for functions that are being debugged, i.e. have break points.
https://codereview.chromium.org/2707873002/diff/710001/src/compiler/bytecode-... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/2707873002/diff/710001/src/compiler/bytecode-... src/compiler/bytecode-graph-builder.cc:816: Node* node = NewNode(op, name, value); Do we ever want to optimize the CollectTypeProfile operator? Otherwise I would vote for just using javascript()->CallRuntime(Runtime::kCollectTypeProfile) directly here instead. Introducing a new opcode only makes sense if some reducer actually handles it specifically.
https://codereview.chromium.org/2707873002/diff/710001/src/compiler/bytecode-... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/2707873002/diff/710001/src/compiler/bytecode-... src/compiler/bytecode-graph-builder.cc:816: Node* node = NewNode(op, name, value); On 2017/03/14 13:12:43, Michael Starzinger wrote: > Do we ever want to optimize the CollectTypeProfile operator? Otherwise I would > vote for just using javascript()->CallRuntime(Runtime::kCollectTypeProfile) > directly here instead. Introducing a new opcode only makes sense if some reducer > actually handles it specifically. We might want to optimize it in the future. But for now maybe we should keep it simple.
https://codereview.chromium.org/2707873002/diff/710001/src/compiler/bytecode-... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/2707873002/diff/710001/src/compiler/bytecode-... src/compiler/bytecode-graph-builder.cc:816: Node* node = NewNode(op, name, value); On 2017/03/14 13:35:22, Yang wrote: > On 2017/03/14 13:12:43, Michael Starzinger wrote: > > Do we ever want to optimize the CollectTypeProfile operator? Otherwise I would > > vote for just using javascript()->CallRuntime(Runtime::kCollectTypeProfile) > > directly here instead. Introducing a new opcode only makes sense if some > reducer > > actually handles it specifically. > > We might want to optimize it in the future. But for now maybe we should keep it > simple. +1 to keeping it simple in the TurboFan IR for now until we have concrete plans. Otherwise approach looks good.
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2707873002/diff/710001/src/compiler/bytecode-... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/2707873002/diff/710001/src/compiler/bytecode-... src/compiler/bytecode-graph-builder.cc:816: Node* node = NewNode(op, name, value); On 2017/03/14 13:41:38, Michael Starzinger wrote: > On 2017/03/14 13:35:22, Yang wrote: > > On 2017/03/14 13:12:43, Michael Starzinger wrote: > > > Do we ever want to optimize the CollectTypeProfile operator? Otherwise I > would > > > vote for just using javascript()->CallRuntime(Runtime::kCollectTypeProfile) > > > directly here instead. Introducing a new opcode only makes sense if some > > reducer > > > actually handles it specifically. > > > > We might want to optimize it in the future. But for now maybe we should keep > it > > simple. > > +1 to keeping it simple in the TurboFan IR for now until we have concrete plans. > Otherwise approach looks good. I deleted the opcode. Please have another look. Thanks!
LGTM on "compiler" and "ast". Thanks! Cool stuff.
The CQ bit was checked by franzih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mvstanton@chromium.org, rmcilroy@chromium.org, yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2707873002/#ps730001 (title: "Delete Opcode.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 730001, "attempt_start_ts": 1489507780102840,
"parent_rev": "d9978e94dcd591c2512484102f145361cad159cb", "commit_rev":
"0332bebde99d0f9a5a8326382f5f37cc26224ae0"}
Message was sent while issue was closed.
Description was changed from ========== Collect type profile for DevTools Collect type information for JavaScript variables and display it in Chrome DevTools. Design Doc: https://docs.google.com/a/google.com/document/d/1O1uepXZXBI6IwiawTrYC3ohhiNgz... When debugging JavaScript, it’s helpful to know the type of a variable, parameter, and return values. JavaScript is dynamically typed, and for complex source code it’s often hard to infer types. With type profiling, we can provide type information to JavaScript developers. This CL is a proof of concept. It collects type profile for assignments and simply prints the types to stdout. The output looks something like this: #my_var1 #Object #number #string #number #undefined #string #Object #Object We use an extra slot in the feedback vector of assignments to carry the list of types for that assignment. The extra slot is only added when the flag --type-profile is given. Missing work: * Collect data for parameters and return values (currently only assignments). * Remove duplicates from the list of collected types and use a common base class. * Add line numbers or source position instead of the variable name. BUG=v8:5935 ========== to ========== Collect type profile for DevTools Collect type information for JavaScript variables and display it in Chrome DevTools. Design Doc: https://docs.google.com/a/google.com/document/d/1O1uepXZXBI6IwiawTrYC3ohhiNgz... When debugging JavaScript, it’s helpful to know the type of a variable, parameter, and return values. JavaScript is dynamically typed, and for complex source code it’s often hard to infer types. With type profiling, we can provide type information to JavaScript developers. This CL is a proof of concept. It collects type profile for assignments and simply prints the types to stdout. The output looks something like this: #my_var1 #Object #number #string #number #undefined #string #Object #Object We use an extra slot in the feedback vector of assignments to carry the list of types for that assignment. The extra slot is only added when the flag --type-profile is given. Missing work: * Collect data for parameters and return values (currently only assignments). * Remove duplicates from the list of collected types and use a common base class. * Add line numbers or source position instead of the variable name. BUG=v8:5935 Review-Url: https://codereview.chromium.org/2707873002 Cr-Commit-Position: refs/heads/master@{#43791} Committed: https://chromium.googlesource.com/v8/v8/+/0332bebde99d0f9a5a8326382f5f37cc262... ==========
Message was sent while issue was closed.
Committed patchset #39 (id:730001) as https://chromium.googlesource.com/v8/v8/+/0332bebde99d0f9a5a8326382f5f37cc262...
Message was sent while issue was closed.
A revert of this CL (patchset #39 id:730001) has been created in https://codereview.chromium.org/2749673003/ by machenbach@chromium.org. The reason for reverting is: gcc bot has problems with this: https://build.chromium.org/p/client.v8/builders/V8%20Linux%20gcc%204.8/builds....
Message was sent while issue was closed.
Description was changed from ========== Collect type profile for DevTools Collect type information for JavaScript variables and display it in Chrome DevTools. Design Doc: https://docs.google.com/a/google.com/document/d/1O1uepXZXBI6IwiawTrYC3ohhiNgz... When debugging JavaScript, it’s helpful to know the type of a variable, parameter, and return values. JavaScript is dynamically typed, and for complex source code it’s often hard to infer types. With type profiling, we can provide type information to JavaScript developers. This CL is a proof of concept. It collects type profile for assignments and simply prints the types to stdout. The output looks something like this: #my_var1 #Object #number #string #number #undefined #string #Object #Object We use an extra slot in the feedback vector of assignments to carry the list of types for that assignment. The extra slot is only added when the flag --type-profile is given. Missing work: * Collect data for parameters and return values (currently only assignments). * Remove duplicates from the list of collected types and use a common base class. * Add line numbers or source position instead of the variable name. BUG=v8:5935 Review-Url: https://codereview.chromium.org/2707873002 Cr-Commit-Position: refs/heads/master@{#43791} Committed: https://chromium.googlesource.com/v8/v8/+/0332bebde99d0f9a5a8326382f5f37cc262... ========== to ========== Collect type profile for DevTools Collect type information for JavaScript variables and display it in Chrome DevTools. Design Doc: https://docs.google.com/a/google.com/document/d/1O1uepXZXBI6IwiawTrYC3ohhiNgz... When debugging JavaScript, it’s helpful to know the type of a variable, parameter, and return values. JavaScript is dynamically typed, and for complex source code it’s often hard to infer types. With type profiling, we can provide type information to JavaScript developers. This CL is a proof of concept. It collects type profile for assignments and simply prints the types to stdout. The output looks something like this: #my_var1 #Object #number #string #number #undefined #string #Object #Object We use an extra slot in the feedback vector of assignments to carry the list of types for that assignment. The extra slot is only added when the flag --type-profile is given. Missing work: * Collect data for parameters and return values (currently only assignments). * Remove duplicates from the list of collected types and use a common base class. * Add line numbers or source position instead of the variable name. BUG=v8:5935 Review-Url: https://codereview.chromium.org/2707873002 Cr-Commit-Position: refs/heads/master@{#43791} Committed: https://chromium.googlesource.com/v8/v8/+/0332bebde99d0f9a5a8326382f5f37cc262... ==========
The CQ bit was checked by franzih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mvstanton@chromium.org, rmcilroy@chromium.org, mstarzinger@chromium.org, yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2707873002/#ps750001 (title: "Use ToCString() instead of debug Print.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 750001, "attempt_start_ts": 1489524175226860,
"parent_rev": "8c1af20baf74cfa286833fb4b4b67b31ad34662b", "commit_rev":
"6cf880f4b84c533d4bb139d33c1369e309d1c579"}
Message was sent while issue was closed.
Description was changed from ========== Collect type profile for DevTools Collect type information for JavaScript variables and display it in Chrome DevTools. Design Doc: https://docs.google.com/a/google.com/document/d/1O1uepXZXBI6IwiawTrYC3ohhiNgz... When debugging JavaScript, it’s helpful to know the type of a variable, parameter, and return values. JavaScript is dynamically typed, and for complex source code it’s often hard to infer types. With type profiling, we can provide type information to JavaScript developers. This CL is a proof of concept. It collects type profile for assignments and simply prints the types to stdout. The output looks something like this: #my_var1 #Object #number #string #number #undefined #string #Object #Object We use an extra slot in the feedback vector of assignments to carry the list of types for that assignment. The extra slot is only added when the flag --type-profile is given. Missing work: * Collect data for parameters and return values (currently only assignments). * Remove duplicates from the list of collected types and use a common base class. * Add line numbers or source position instead of the variable name. BUG=v8:5935 Review-Url: https://codereview.chromium.org/2707873002 Cr-Commit-Position: refs/heads/master@{#43791} Committed: https://chromium.googlesource.com/v8/v8/+/0332bebde99d0f9a5a8326382f5f37cc262... ========== to ========== Collect type profile for DevTools Collect type information for JavaScript variables and display it in Chrome DevTools. Design Doc: https://docs.google.com/a/google.com/document/d/1O1uepXZXBI6IwiawTrYC3ohhiNgz... When debugging JavaScript, it’s helpful to know the type of a variable, parameter, and return values. JavaScript is dynamically typed, and for complex source code it’s often hard to infer types. With type profiling, we can provide type information to JavaScript developers. This CL is a proof of concept. It collects type profile for assignments and simply prints the types to stdout. The output looks something like this: #my_var1 #Object #number #string #number #undefined #string #Object #Object We use an extra slot in the feedback vector of assignments to carry the list of types for that assignment. The extra slot is only added when the flag --type-profile is given. Missing work: * Collect data for parameters and return values (currently only assignments). * Remove duplicates from the list of collected types and use a common base class. * Add line numbers or source position instead of the variable name. BUG=v8:5935 Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Commit-Position: refs/heads/master@{#43791} Committed: https://chromium.googlesource.com/v8/v8/+/0332bebde99d0f9a5a8326382f5f37cc262... Review-Url: https://codereview.chromium.org/2707873002 Cr-Commit-Position: refs/heads/master@{#43804} Committed: https://chromium.googlesource.com/v8/v8/+/6cf880f4b84c533d4bb139d33c1369e309d... ==========
Message was sent while issue was closed.
Committed patchset #40 (id:750001) as https://chromium.googlesource.com/v8/v8/+/6cf880f4b84c533d4bb139d33c1369e309d...
Message was sent while issue was closed.
Hmmm, now the bot turned flaky: https://build.chromium.org/p/client.v8/builders/V8%20Linux%20gcc%204.8/builds...
Message was sent while issue was closed.
A revert of this CL (patchset #40 id:750001) has been created in https://codereview.chromium.org/2754573002/ by franzih@chromium.org. The reason for reverting is: gcc bot is now flaky https://build.chromium.org/p/client.v8/builders/V8%20Linux%20gcc%204.8/builds....
Message was sent while issue was closed.
Description was changed from ========== Collect type profile for DevTools Collect type information for JavaScript variables and display it in Chrome DevTools. Design Doc: https://docs.google.com/a/google.com/document/d/1O1uepXZXBI6IwiawTrYC3ohhiNgz... When debugging JavaScript, it’s helpful to know the type of a variable, parameter, and return values. JavaScript is dynamically typed, and for complex source code it’s often hard to infer types. With type profiling, we can provide type information to JavaScript developers. This CL is a proof of concept. It collects type profile for assignments and simply prints the types to stdout. The output looks something like this: #my_var1 #Object #number #string #number #undefined #string #Object #Object We use an extra slot in the feedback vector of assignments to carry the list of types for that assignment. The extra slot is only added when the flag --type-profile is given. Missing work: * Collect data for parameters and return values (currently only assignments). * Remove duplicates from the list of collected types and use a common base class. * Add line numbers or source position instead of the variable name. BUG=v8:5935 Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Commit-Position: refs/heads/master@{#43791} Committed: https://chromium.googlesource.com/v8/v8/+/0332bebde99d0f9a5a8326382f5f37cc262... Review-Url: https://codereview.chromium.org/2707873002 Cr-Commit-Position: refs/heads/master@{#43804} Committed: https://chromium.googlesource.com/v8/v8/+/6cf880f4b84c533d4bb139d33c1369e309d... ========== to ========== Collect type profile for DevTools Collect type information for JavaScript variables and display it in Chrome DevTools. Design Doc: https://docs.google.com/a/google.com/document/d/1O1uepXZXBI6IwiawTrYC3ohhiNgz... When debugging JavaScript, it’s helpful to know the type of a variable, parameter, and return values. JavaScript is dynamically typed, and for complex source code it’s often hard to infer types. With type profiling, we can provide type information to JavaScript developers. This CL is a proof of concept. It collects type profile for assignments and simply prints the types to stdout. The output looks something like this: #my_var1 #Object #number #string #number #undefined #string #Object #Object We use an extra slot in the feedback vector of assignments to carry the list of types for that assignment. The extra slot is only added when the flag --type-profile is given. Missing work: * Collect data for parameters and return values (currently only assignments). * Remove duplicates from the list of collected types and use a common base class. * Add line numbers or source position instead of the variable name. BUG=v8:5935 Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Commit-Position: refs/heads/master@{#43791} Committed: https://chromium.googlesource.com/v8/v8/+/0332bebde99d0f9a5a8326382f5f37cc262... Review-Url: https://codereview.chromium.org/2707873002 Cr-Commit-Position: refs/heads/master@{#43804} Committed: https://chromium.googlesource.com/v8/v8/+/6cf880f4b84c533d4bb139d33c1369e309d... ==========
The CQ bit was checked by franzih@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 750001, "attempt_start_ts": 1489652200151920,
"parent_rev": "b16630cfd41f9bef5ed3ed505b13fcac29bb3ea2", "commit_rev":
"5c322873908a5b5c04552fc47d8d81f7603b5d11"}
Message was sent while issue was closed.
Description was changed from ========== Collect type profile for DevTools Collect type information for JavaScript variables and display it in Chrome DevTools. Design Doc: https://docs.google.com/a/google.com/document/d/1O1uepXZXBI6IwiawTrYC3ohhiNgz... When debugging JavaScript, it’s helpful to know the type of a variable, parameter, and return values. JavaScript is dynamically typed, and for complex source code it’s often hard to infer types. With type profiling, we can provide type information to JavaScript developers. This CL is a proof of concept. It collects type profile for assignments and simply prints the types to stdout. The output looks something like this: #my_var1 #Object #number #string #number #undefined #string #Object #Object We use an extra slot in the feedback vector of assignments to carry the list of types for that assignment. The extra slot is only added when the flag --type-profile is given. Missing work: * Collect data for parameters and return values (currently only assignments). * Remove duplicates from the list of collected types and use a common base class. * Add line numbers or source position instead of the variable name. BUG=v8:5935 Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Commit-Position: refs/heads/master@{#43791} Committed: https://chromium.googlesource.com/v8/v8/+/0332bebde99d0f9a5a8326382f5f37cc262... Review-Url: https://codereview.chromium.org/2707873002 Cr-Commit-Position: refs/heads/master@{#43804} Committed: https://chromium.googlesource.com/v8/v8/+/6cf880f4b84c533d4bb139d33c1369e309d... ========== to ========== Collect type profile for DevTools Collect type information for JavaScript variables and display it in Chrome DevTools. Design Doc: https://docs.google.com/a/google.com/document/d/1O1uepXZXBI6IwiawTrYC3ohhiNgz... When debugging JavaScript, it’s helpful to know the type of a variable, parameter, and return values. JavaScript is dynamically typed, and for complex source code it’s often hard to infer types. With type profiling, we can provide type information to JavaScript developers. This CL is a proof of concept. It collects type profile for assignments and simply prints the types to stdout. The output looks something like this: #my_var1 #Object #number #string #number #undefined #string #Object #Object We use an extra slot in the feedback vector of assignments to carry the list of types for that assignment. The extra slot is only added when the flag --type-profile is given. Missing work: * Collect data for parameters and return values (currently only assignments). * Remove duplicates from the list of collected types and use a common base class. * Add line numbers or source position instead of the variable name. BUG=v8:5935 Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Original-Commit-Position: refs/heads/master@{#43791} Committed: https://chromium.googlesource.com/v8/v8/+/0332bebde99d0f9a5a8326382f5f37cc262... Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Commit-Position: refs/heads/master@{#43804} Committed: https://chromium.googlesource.com/v8/v8/+/6cf880f4b84c533d4bb139d33c1369e309d... Review-Url: https://codereview.chromium.org/2707873002 Cr-Commit-Position: refs/heads/master@{#43846} Committed: https://chromium.googlesource.com/v8/v8/+/5c322873908a5b5c04552fc47d8d81f7603... ==========
Message was sent while issue was closed.
Committed patchset #40 (id:750001) as https://chromium.googlesource.com/v8/v8/+/5c322873908a5b5c04552fc47d8d81f7603...
Message was sent while issue was closed.
A revert of this CL (patchset #40 id:750001) has been created in https://codereview.chromium.org/2747383004/ by franzih@chromium.org. The reason for reverting is: Flaky under stress. Fix first. .
Message was sent while issue was closed.
Description was changed from ========== Collect type profile for DevTools Collect type information for JavaScript variables and display it in Chrome DevTools. Design Doc: https://docs.google.com/a/google.com/document/d/1O1uepXZXBI6IwiawTrYC3ohhiNgz... When debugging JavaScript, it’s helpful to know the type of a variable, parameter, and return values. JavaScript is dynamically typed, and for complex source code it’s often hard to infer types. With type profiling, we can provide type information to JavaScript developers. This CL is a proof of concept. It collects type profile for assignments and simply prints the types to stdout. The output looks something like this: #my_var1 #Object #number #string #number #undefined #string #Object #Object We use an extra slot in the feedback vector of assignments to carry the list of types for that assignment. The extra slot is only added when the flag --type-profile is given. Missing work: * Collect data for parameters and return values (currently only assignments). * Remove duplicates from the list of collected types and use a common base class. * Add line numbers or source position instead of the variable name. BUG=v8:5935 Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Original-Commit-Position: refs/heads/master@{#43791} Committed: https://chromium.googlesource.com/v8/v8/+/0332bebde99d0f9a5a8326382f5f37cc262... Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Commit-Position: refs/heads/master@{#43804} Committed: https://chromium.googlesource.com/v8/v8/+/6cf880f4b84c533d4bb139d33c1369e309d... Review-Url: https://codereview.chromium.org/2707873002 Cr-Commit-Position: refs/heads/master@{#43846} Committed: https://chromium.googlesource.com/v8/v8/+/5c322873908a5b5c04552fc47d8d81f7603... ========== to ========== Collect type profile for DevTools Collect type information for JavaScript variables and display it in Chrome DevTools. Design Doc: https://docs.google.com/a/google.com/document/d/1O1uepXZXBI6IwiawTrYC3ohhiNgz... When debugging JavaScript, it’s helpful to know the type of a variable, parameter, and return values. JavaScript is dynamically typed, and for complex source code it’s often hard to infer types. With type profiling, we can provide type information to JavaScript developers. This CL is a proof of concept. It collects type profile for assignments and simply prints the types to stdout. The output looks something like this: #my_var1 #Object #number #string #number #undefined #string #Object #Object We use an extra slot in the feedback vector of assignments to carry the list of types for that assignment. The extra slot is only added when the flag --type-profile is given. Missing work: * Collect data for parameters and return values (currently only assignments). * Remove duplicates from the list of collected types and use a common base class. * Add line numbers or source position instead of the variable name. BUG=v8:5935 Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Original-Commit-Position: refs/heads/master@{#43791} Committed: https://chromium.googlesource.com/v8/v8/+/0332bebde99d0f9a5a8326382f5f37cc262... Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Commit-Position: refs/heads/master@{#43804} Committed: https://chromium.googlesource.com/v8/v8/+/6cf880f4b84c533d4bb139d33c1369e309d... Review-Url: https://codereview.chromium.org/2707873002 Cr-Commit-Position: refs/heads/master@{#43846} Committed: https://chromium.googlesource.com/v8/v8/+/5c322873908a5b5c04552fc47d8d81f7603... ==========
The CQ bit was checked by franzih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mvstanton@chromium.org, rmcilroy@chromium.org, mstarzinger@chromium.org, yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2707873002/#ps770001 (title: "Collect type profile only for user JS.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 770001, "attempt_start_ts": 1489654856477370,
"parent_rev": "460ba9c22405d839a642eb2ba799ab2bcc611940", "commit_rev":
"18c35e4958be6a70acc923bf10363eb9aaee5ce4"}
Message was sent while issue was closed.
Description was changed from ========== Collect type profile for DevTools Collect type information for JavaScript variables and display it in Chrome DevTools. Design Doc: https://docs.google.com/a/google.com/document/d/1O1uepXZXBI6IwiawTrYC3ohhiNgz... When debugging JavaScript, it’s helpful to know the type of a variable, parameter, and return values. JavaScript is dynamically typed, and for complex source code it’s often hard to infer types. With type profiling, we can provide type information to JavaScript developers. This CL is a proof of concept. It collects type profile for assignments and simply prints the types to stdout. The output looks something like this: #my_var1 #Object #number #string #number #undefined #string #Object #Object We use an extra slot in the feedback vector of assignments to carry the list of types for that assignment. The extra slot is only added when the flag --type-profile is given. Missing work: * Collect data for parameters and return values (currently only assignments). * Remove duplicates from the list of collected types and use a common base class. * Add line numbers or source position instead of the variable name. BUG=v8:5935 Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Original-Commit-Position: refs/heads/master@{#43791} Committed: https://chromium.googlesource.com/v8/v8/+/0332bebde99d0f9a5a8326382f5f37cc262... Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Commit-Position: refs/heads/master@{#43804} Committed: https://chromium.googlesource.com/v8/v8/+/6cf880f4b84c533d4bb139d33c1369e309d... Review-Url: https://codereview.chromium.org/2707873002 Cr-Commit-Position: refs/heads/master@{#43846} Committed: https://chromium.googlesource.com/v8/v8/+/5c322873908a5b5c04552fc47d8d81f7603... ========== to ========== Collect type profile for DevTools Collect type information for JavaScript variables and display it in Chrome DevTools. Design Doc: https://docs.google.com/a/google.com/document/d/1O1uepXZXBI6IwiawTrYC3ohhiNgz... When debugging JavaScript, it’s helpful to know the type of a variable, parameter, and return values. JavaScript is dynamically typed, and for complex source code it’s often hard to infer types. With type profiling, we can provide type information to JavaScript developers. This CL is a proof of concept. It collects type profile for assignments and simply prints the types to stdout. The output looks something like this: #my_var1 #Object #number #string #number #undefined #string #Object #Object We use an extra slot in the feedback vector of assignments to carry the list of types for that assignment. The extra slot is only added when the flag --type-profile is given. Missing work: * Collect data for parameters and return values (currently only assignments). * Remove duplicates from the list of collected types and use a common base class. * Add line numbers or source position instead of the variable name. BUG=v8:5935 Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Original-Original-Commit-Position: refs/heads/master@{#43791} Committed: https://chromium.googlesource.com/v8/v8/+/0332bebde99d0f9a5a8326382f5f37cc262... Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Original-Commit-Position: refs/heads/master@{#43804} Committed: https://chromium.googlesource.com/v8/v8/+/6cf880f4b84c533d4bb139d33c1369e309d... Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Commit-Position: refs/heads/master@{#43846} Committed: https://chromium.googlesource.com/v8/v8/+/5c322873908a5b5c04552fc47d8d81f7603... Review-Url: https://codereview.chromium.org/2707873002 Cr-Commit-Position: refs/heads/master@{#43849} Committed: https://chromium.googlesource.com/v8/v8/+/18c35e4958be6a70acc923bf10363eb9aae... ==========
Message was sent while issue was closed.
Committed patchset #41 (id:770001) as https://chromium.googlesource.com/v8/v8/+/18c35e4958be6a70acc923bf10363eb9aae...
Message was sent while issue was closed.
A revert of this CL (patchset #41 id:770001) has been created in https://codereview.chromium.org/2745413006/ by franzih@chromium.org. The reason for reverting is: Still flaky.
Message was sent while issue was closed.
Description was changed from ========== Collect type profile for DevTools Collect type information for JavaScript variables and display it in Chrome DevTools. Design Doc: https://docs.google.com/a/google.com/document/d/1O1uepXZXBI6IwiawTrYC3ohhiNgz... When debugging JavaScript, it’s helpful to know the type of a variable, parameter, and return values. JavaScript is dynamically typed, and for complex source code it’s often hard to infer types. With type profiling, we can provide type information to JavaScript developers. This CL is a proof of concept. It collects type profile for assignments and simply prints the types to stdout. The output looks something like this: #my_var1 #Object #number #string #number #undefined #string #Object #Object We use an extra slot in the feedback vector of assignments to carry the list of types for that assignment. The extra slot is only added when the flag --type-profile is given. Missing work: * Collect data for parameters and return values (currently only assignments). * Remove duplicates from the list of collected types and use a common base class. * Add line numbers or source position instead of the variable name. BUG=v8:5935 Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Original-Original-Commit-Position: refs/heads/master@{#43791} Committed: https://chromium.googlesource.com/v8/v8/+/0332bebde99d0f9a5a8326382f5f37cc262... Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Original-Commit-Position: refs/heads/master@{#43804} Committed: https://chromium.googlesource.com/v8/v8/+/6cf880f4b84c533d4bb139d33c1369e309d... Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Commit-Position: refs/heads/master@{#43846} Committed: https://chromium.googlesource.com/v8/v8/+/5c322873908a5b5c04552fc47d8d81f7603... Review-Url: https://codereview.chromium.org/2707873002 Cr-Commit-Position: refs/heads/master@{#43849} Committed: https://chromium.googlesource.com/v8/v8/+/18c35e4958be6a70acc923bf10363eb9aae... ========== to ========== Collect type profile for DevTools Collect type information for JavaScript variables and display it in Chrome DevTools. Design Doc: https://docs.google.com/a/google.com/document/d/1O1uepXZXBI6IwiawTrYC3ohhiNgz... When debugging JavaScript, it’s helpful to know the type of a variable, parameter, and return values. JavaScript is dynamically typed, and for complex source code it’s often hard to infer types. With type profiling, we can provide type information to JavaScript developers. This CL is a proof of concept. It collects type profile for assignments and simply prints the types to stdout. The output looks something like this: #my_var1 #Object #number #string #number #undefined #string #Object #Object We use an extra slot in the feedback vector of assignments to carry the list of types for that assignment. The extra slot is only added when the flag --type-profile is given. Missing work: * Collect data for parameters and return values (currently only assignments). * Remove duplicates from the list of collected types and use a common base class. * Add line numbers or source position instead of the variable name. BUG=v8:5935 Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Original-Original-Commit-Position: refs/heads/master@{#43791} Committed: https://chromium.googlesource.com/v8/v8/+/0332bebde99d0f9a5a8326382f5f37cc262... Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Original-Commit-Position: refs/heads/master@{#43804} Committed: https://chromium.googlesource.com/v8/v8/+/6cf880f4b84c533d4bb139d33c1369e309d... Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Commit-Position: refs/heads/master@{#43846} Committed: https://chromium.googlesource.com/v8/v8/+/5c322873908a5b5c04552fc47d8d81f7603... Review-Url: https://codereview.chromium.org/2707873002 Cr-Commit-Position: refs/heads/master@{#43849} Committed: https://chromium.googlesource.com/v8/v8/+/18c35e4958be6a70acc923bf10363eb9aae... ==========
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...) v8_win_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Collect type profile for DevTools Collect type information for JavaScript variables and display it in Chrome DevTools. Design Doc: https://docs.google.com/a/google.com/document/d/1O1uepXZXBI6IwiawTrYC3ohhiNgz... When debugging JavaScript, it’s helpful to know the type of a variable, parameter, and return values. JavaScript is dynamically typed, and for complex source code it’s often hard to infer types. With type profiling, we can provide type information to JavaScript developers. This CL is a proof of concept. It collects type profile for assignments and simply prints the types to stdout. The output looks something like this: #my_var1 #Object #number #string #number #undefined #string #Object #Object We use an extra slot in the feedback vector of assignments to carry the list of types for that assignment. The extra slot is only added when the flag --type-profile is given. Missing work: * Collect data for parameters and return values (currently only assignments). * Remove duplicates from the list of collected types and use a common base class. * Add line numbers or source position instead of the variable name. BUG=v8:5935 Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Original-Original-Commit-Position: refs/heads/master@{#43791} Committed: https://chromium.googlesource.com/v8/v8/+/0332bebde99d0f9a5a8326382f5f37cc262... Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Original-Commit-Position: refs/heads/master@{#43804} Committed: https://chromium.googlesource.com/v8/v8/+/6cf880f4b84c533d4bb139d33c1369e309d... Review-Url: https://codereview.chromium.org/2707873002 Cr-Original-Commit-Position: refs/heads/master@{#43846} Committed: https://chromium.googlesource.com/v8/v8/+/5c322873908a5b5c04552fc47d8d81f7603... Review-Url: https://codereview.chromium.org/2707873002 Cr-Commit-Position: refs/heads/master@{#43849} Committed: https://chromium.googlesource.com/v8/v8/+/18c35e4958be6a70acc923bf10363eb9aae... ========== to ========== Collect type profile for DevTools Collect type information for JavaScript variables and display it in Chrome DevTools. Design Doc: https://docs.google.com/a/google.com/document/d/1O1uepXZXBI6IwiawTrYC3ohhiNgz... When debugging JavaScript, it’s helpful to know the type of a variable, parameter, and return values. JavaScript is dynamically typed, and for complex source code it’s often hard to infer types. With type profiling, we can provide type information to JavaScript developers. This CL is a proof of concept. It collects type profile for assignments and simply prints the types to stdout. The output looks something like this: #my_var1 #Object #number #string #number #undefined #string #Object #Object We use an extra slot in the feedback vector of assignments to carry the list of types for that assignment. The extra slot is only added when the flag --type-profile is given. Missing work: * Collect data for parameters and return values (currently only assignments). * Remove duplicates from the list of collected types and use a common base class. * Add line numbers or source position instead of the variable name. For now, has a test that compares the stdout of --type-profile in test/message. We will remove this test when --type-profile is fully integrated in the debugger protocol. Adding the test in test/inspector does not work, because the inspector test itself consists of JavaScript code that would convolute the output and be non-deterministic under stress. BUG=v8:5935 ==========
The CQ bit was checked by franzih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by franzih@chromium.org
The CQ bit was checked by franzih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mvstanton@chromium.org, rmcilroy@chromium.org, mstarzinger@chromium.org, yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2707873002/#ps870001 (title: "Explain why throw is needed in message test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 870001, "attempt_start_ts": 1489672727815070,
"parent_rev": "a4c73fa704f5015fc259e3e266f72340e5f0ae1a", "commit_rev":
"947a0437668ce21392358634494543a664c6b123"}
Message was sent while issue was closed.
Description was changed from ========== Collect type profile for DevTools Collect type information for JavaScript variables and display it in Chrome DevTools. Design Doc: https://docs.google.com/a/google.com/document/d/1O1uepXZXBI6IwiawTrYC3ohhiNgz... When debugging JavaScript, it’s helpful to know the type of a variable, parameter, and return values. JavaScript is dynamically typed, and for complex source code it’s often hard to infer types. With type profiling, we can provide type information to JavaScript developers. This CL is a proof of concept. It collects type profile for assignments and simply prints the types to stdout. The output looks something like this: #my_var1 #Object #number #string #number #undefined #string #Object #Object We use an extra slot in the feedback vector of assignments to carry the list of types for that assignment. The extra slot is only added when the flag --type-profile is given. Missing work: * Collect data for parameters and return values (currently only assignments). * Remove duplicates from the list of collected types and use a common base class. * Add line numbers or source position instead of the variable name. For now, has a test that compares the stdout of --type-profile in test/message. We will remove this test when --type-profile is fully integrated in the debugger protocol. Adding the test in test/inspector does not work, because the inspector test itself consists of JavaScript code that would convolute the output and be non-deterministic under stress. BUG=v8:5935 ========== to ========== Collect type profile for DevTools Collect type information for JavaScript variables and display it in Chrome DevTools. Design Doc: https://docs.google.com/a/google.com/document/d/1O1uepXZXBI6IwiawTrYC3ohhiNgz... When debugging JavaScript, it’s helpful to know the type of a variable, parameter, and return values. JavaScript is dynamically typed, and for complex source code it’s often hard to infer types. With type profiling, we can provide type information to JavaScript developers. This CL is a proof of concept. It collects type profile for assignments and simply prints the types to stdout. The output looks something like this: #my_var1 #Object #number #string #number #undefined #string #Object #Object We use an extra slot in the feedback vector of assignments to carry the list of types for that assignment. The extra slot is only added when the flag --type-profile is given. Missing work: * Collect data for parameters and return values (currently only assignments). * Remove duplicates from the list of collected types and use a common base class. * Add line numbers or source position instead of the variable name. For now, has a test that compares the stdout of --type-profile in test/message. We will remove this test when --type-profile is fully integrated in the debugger protocol. Adding the test in test/inspector does not work, because the inspector test itself consists of JavaScript code that would convolute the output and be non-deterministic under stress. BUG=v8:5935 Review-Url: https://codereview.chromium.org/2707873002 Cr-Commit-Position: refs/heads/master@{#43866} Committed: https://chromium.googlesource.com/v8/v8/+/947a0437668ce21392358634494543a664c... ==========
Message was sent while issue was closed.
Committed patchset #46 (id:870001) as https://chromium.googlesource.com/v8/v8/+/947a0437668ce21392358634494543a664c... |
