Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Experimental idea - create facility to enable internal commands within JS #6440

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

rhuanjl
Copy link
Collaborator

@rhuanjl rhuanjl commented May 1, 2020

This PR introduces the concept of an internal command, something that could be used in self-hosted JS and in testing environments to pass a command to the engine within a JS file that is not normal JS.

This is illustrated with an example that allows the use of the bytecode operation for object coercion within the JsBuiltins instead of calling the object constructor to accomplish the same.

Broader use of this feature would require implementing specific commands for it to make use of which could include both facilities for JsBuiltins we don't wish to use runtime methods for AND potentially testing features.

Opened as a draft PR at the moment for comment consideration - aware it would need some tidy up pre-merge; also should probably not be merged without a bigger use case than object coercion.

Currently this does the folowing:

  1. Introduce a flag -EnableInternalCommands accepted only in debug and test builds
  2. Implement in the parser a facility to detect an internal command marked by @@ within a script, currently it can recognise @@Conv_Obj and @@Conv_Num - though the later is useless (it's resultant bytecode is identical to a unary plus) .
  3. Implement in the bytecode emitter a facility to emit bytecode for these internal commands. (Note currently this only supports a single parameter and should be updated to support multiple parameters if the feature is to be expanded and used)

@rhuanjl rhuanjl force-pushed the inject branch 3 times, most recently from 1c4ede3 to e58e250 Compare May 2, 2020 12:44
@rhuanjl
Copy link
Collaborator Author

rhuanjl commented May 8, 2020

I've done some significant reworking on this:

  1. Created InternalCommands.h to hold the list of internal commands so they don't have to be listed in several places as they originally did.
  2. Added support for variable numbers of parameters in the parser and 1 or 2 in the bytecode emitter with errors are parse time for incorrect parameter counts. Note all current uses only have 1 parameter.
  3. Removed @@Conv_Num which whilst useful for testing this mechanism is useless in practise as a unary plus does the same.
  4. Implemented ToInteger, ToLength and GetLength as new bytecode OpCodes and InternalCommands
  5. Implemented ToInteger, ToLength and GetLength in the JIT as helper calls AND in the optimising phases:
    1. ToInteger gets removed if its source is type specified as an integer.
    2. ToLength gets removed if its source is type specified as a positive integer.
    3. GetLength gets converted into a direct length access for array like objects excluding typedarrays.
  6. Updated JsBuiltin.js to use these new internal commands - and removed the ChakraLib methods that used to be used.

Testing with ARES-6 in the benchmarks folder this seems to show some significant improvement ~8% gain - though could likely do with some more extensive testing to see how consistent that is.

I'm not 100% if I've done the Jit parts right - I think I have but could do with someone who understands the Jit a little better giving it a look over.

Open Questions

  1. This is obviously intended as a starting point for more internal commands like this AND also for more Js builtins - so probably still need some consideration if that's a direction we want to take.
  2. @@ is illegal syntax in javascript - that makes it ideal for unambiguous implementation in the parser - BUT - it means that syntax highlighting will break when writing code that uses it. It may be good to find an alternative that is technically legal syntax BUT that we'll never want to use in any internal code, the best idea I can think of is !! or even !!!- any thoughts on this?

@rhuanjl rhuanjl force-pushed the inject branch 2 times, most recently from 50d5555 to 7ac45e4 Compare May 8, 2020 13:59
@rhuanjl
Copy link
Collaborator Author

rhuanjl commented May 8, 2020

Note I've added a largely unrelated change:
The array sort test was timing out - this shouldn't have been significantly impacted by this PR - it is timing out on my readme update as well. To stop it timing out I've added a commit that lowers the arbitrary threshold for switching from Insertion sort to merge sort and updated the test so that it shouldn't run for as long.

This test was always on the boundary of being too slow - though I would like to look at further optimisations to array.sort for long arrays.

@rhuanjl
Copy link
Collaborator Author

rhuanjl commented May 15, 2020

Giving this a bit more thought - one thing it needs which it currently doesn't have is specific tests for these internal commands including putting them through:
a) weird options/cases where they'll have to handled abnormal values AND
b) any cases that should trigger JIT optimisations - with some way to check that these work

@rhuanjl
Copy link
Collaborator Author

rhuanjl commented May 16, 2020

Whilst working on tests I've reviewed a few scenarios - my jit handling needs more work, particularly for the GetLength op.

@rhuanjl
Copy link
Collaborator Author

rhuanjl commented Jun 4, 2020

While I intend to revisit this it is not planned for our first community release, so not planning to merge it this month - leaving open as draft.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant