Closed
Bug 1478034
Opened 7 years ago
Closed 4 months ago
[meta] Add an interface to bytecode
Categories
(Core :: JavaScript Engine, enhancement, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox63 | --- | affected |
People
(Reporter: mgaudet, Unassigned)
References
(Depends on 1 open bug)
Details
(Keywords: meta)
Attachments
(1 file, 2 obsolete files)
2.35 KB,
text/plain
|
Details |
Currently access into the bytecode often happen relatively loosely, and many parts of the VM look directly at bytecode. This is a tight coupling that reduces the agility of the team, making experimentation and potential evolution more expensive that necessary.
A bytecode interface can raise the level of discussion about program text to semantics, rather than structure.
Design Goals:
* Provide a framework to write semantic queries about bytecode, without exposing bytecode specifics.
* Ease of iteration and analysis
* Encapsulation: longer term only very specific clients outside the interface should directly reference the bytecode (jsbytecode*, pcOffset).
Reporter | ||
Comment 1•7 years ago
|
||
This is a functional first draft of an inteface for bytecode. There are xxx
major portions included in this patch.
- RawBytecode: A typedef to expose jsbytecode* in a controlled manner, to allow
easy future audits.
- ScriptLocation: This is an immutable view of a single program operation (a
bytecode). There is is some basic functionality in here included for
experimentation sake.
- ScriptLocationOffset: An immutable view of a source location offset, (and
RawscriptLocationOffset, another typedef to ease future auditing)
- ScriptIterator: A forward iterator over script locations, using program
text order.
- AllBytecodeIterable: A wrapper class that allows the use of a range-based
for-loop over ScriptLocations in a Script.
- Modifications to JSScript to expose ScriptLocations and answer some queries
about containment.
The interface is experimentally tested using JSScript::initScriptCounts as a
simple playground for demonstrating the use of the interface.
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → mgaudet
Status: NEW → ASSIGNED
Reporter | ||
Comment 2•7 years ago
|
||
The interface is expanded and refined in this patch, showing a more
complicated set of requirements and the answers to those requirements
Reporter | ||
Comment 3•7 years ago
|
||
Comment on attachment 8994540 [details] [diff] [review]
[Part 1] Strawman implementation of a bytecode interface
I'd welcome suggestions for anyone else to rope in on feedback!
Attachment #8994540 -
Flags: feedback?(tcampbell)
Attachment #8994540 -
Flags: feedback?(kvijayan)
Reporter | ||
Updated•7 years ago
|
Attachment #8994541 -
Flags: feedback?(tcampbell)
Attachment #8994541 -
Flags: feedback?(kvijayan)
Comment 4•7 years ago
|
||
Comment on attachment 8994540 [details] [diff] [review]
[Part 1] Strawman implementation of a bytecode interface
Review of attachment 8994540 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/vm/ScriptIterator.h
@@ +10,5 @@
> +#include "vm/ScriptLocation.h"
> +
> +namespace js {
> +
> +class ScriptIterator
nit: Script Iterator sounds like this would be an Iterator which returns a Script at each step.
just suggesting random ideas: CodePointIterator ? BytecodeIterator? InstructionIterator? OpcodeIterator? …
::: js/src/vm/ScriptLocation.h
@@ +41,5 @@
> +// JSBytecode. However, a space efficient implementation of
> +// an iterator's ::end() seems like it would really want to
> +// point to the invalid location just past the end of the
> +// script.
> +class ScriptLocation
nit: ScriptLocation sounds like it would contain a line & column fields, where it is more like a CodePoint / Bytecode / Instruction / Opcode / … (pick one)
Comment 5•7 years ago
|
||
I did a quick audit of all the places we use a base JSOP_* in the codebase and identified a few common patterns.
* Asserts - Debug-only asserts as sanity checks.
* ParseNode - Some ParseNode use a small number of opcodes as enums.
* Compare ops - Parts of MacroAssembler use a small number of opcodes as enums for basic comparison operations.
* Bytecode emitter - Obviously we need opcodes to generate bytecode.
* Dump internals - Some debug dump information directly pokes into opcodes.
* JIT translation - Our JITs translate from bytecode, so no surprise.
* Bytecode metadata - Helper methods to query metadata about opcode definitions.
* VM Operation - A few VM options use opcodes instead of enum options.
* Pattern match exact bytecode - A few parts of code pattern match exact bytecode patterns. At the very least they should be defined in a common place.
> builtin/Array.cpp
> builtin/RegExp.cpp
> vm/NativeObject.cpp
> vm/ObjectGroup.cpp
* Control flow - A few places inspect control flow opcodes. These probably should be provided a well designed interface.
> vm/CodeCoverage.cpp
> vm/Debugger.cpp
> vm/GeneratorObject.cpp
> vm/GeneratorObject.h
Comment 6•7 years ago
|
||
We should consider the cases that use opcodes as a convenient enum value to instead define their own enum and explictly convert. Assigning same encoding is fine so the conversion is a no-op, but it does better document things and de-couples the bytecode.
Comment 7•7 years ago
|
||
(In reply to Ted Campbell [:tcampbell] from comment #5)
> * Compare ops - Parts of MacroAssembler use a small number of opcodes as
> enums for basic comparison operations.
Oops, the only use in macroassembler is a helper function to convert JSOP_* to a condition code. Maybe these should just be pushed to the JITs.
Reporter | ||
Updated•7 years ago
|
Attachment #8994540 -
Flags: feedback?(tcampbell)
Attachment #8994540 -
Flags: feedback?(kvijayan)
Reporter | ||
Updated•7 years ago
|
Attachment #8994541 -
Flags: feedback?(tcampbell)
Attachment #8994541 -
Flags: feedback?(kvijayan)
Comment 8•7 years ago
|
||
Comment on attachment 8994540 [details] [diff] [review]
[Part 1] Strawman implementation of a bytecode interface
Review of attachment 8994540 [details] [diff] [review]:
-----------------------------------------------------------------
Sorry for taking a while to look at this. Looks like a good start, aside from naming nit. Can we land this somehow and then build some sort of "isbytecodeprivateyet" metric to measure incremental conversion of all usages of jsbytecode* and pcOffset to this interface?
::: js/src/vm/ScriptIterator.h
@@ +10,5 @@
> +#include "vm/ScriptLocation.h"
> +
> +namespace js {
> +
> +class ScriptIterator
I agree with :nbp - ScriptIterator is confusing. BytecodeIterator seems more appropriate.
::: js/src/vm/ScriptLocation.h
@@ +41,5 @@
> +// JSBytecode. However, a space efficient implementation of
> +// an iterator's ::end() seems like it would really want to
> +// point to the invalid location just past the end of the
> +// script.
> +class ScriptLocation
Agree with :nbp again. BytecodeLocation goes along well with BytecodeIterator.
Reporter | ||
Updated•7 years ago
|
Priority: -- → P3
QA Contact: sdetar
Updated•7 years ago
|
QA Contact: sdetar
Reporter | ||
Updated•7 years ago
|
Attachment #8994540 -
Attachment is obsolete: true
Reporter | ||
Updated•7 years ago
|
Attachment #8994541 -
Attachment is obsolete: true
Reporter | ||
Comment 9•7 years ago
|
||
Converting this to a meta-bug
Comment 10•7 years ago
|
||
Comment 11•7 years ago
|
||
Attachment: These are notes from a meeting (I believe in Paris) where we discussed this. Of dubious value, attached just in case.
Updated•6 years ago
|
Summary: Add an interface to bytecode → [meta] Add an interface to bytecode
Updated•3 years ago
|
Severity: normal → S3
Updated•4 months ago
|
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•