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

Specialized bytecode for default constructors #112

Merged
merged 1 commit into from
Nov 24, 2023

Conversation

littledivy
Copy link
Contributor

No description provided.

@littledivy littledivy changed the title Specialized bytecode for generated constructors Specialized bytecode for default constructors Nov 22, 2023
@littledivy littledivy force-pushed the optimize_auto_constructors branch from 7c7eab5 to 40d1fa0 Compare November 22, 2023 11:21
@bnoordhuis
Copy link
Contributor

You must have anticipated I'm going to inquire about benchmark results, right? Don't keep me in suspense! :-)

@littledivy
Copy link
Contributor Author

littledivy commented Nov 22, 2023

Lol, only a very minor improvement executing a file with 1000 classes with default ctor:

// silly_bench.js
class A0 {} 
class B0 extends A0 {}
new B0()
// ...
class A999 {} 
class B999 extends A999 {}
new B999()

Running hyperfine:

Summary
  './build/qjs --stack-size 2000000 bench.js' ran
    1.15 ± 0.80 times faster than './qjs_master --stack-size 2000000 bench.js'

I don't think Octane, SunSpider or Kraken use class {} so nothing changed there either.

Didn't really expect much improvements anyways - bytecode generated is the same as before.

@bnoordhuis
Copy link
Contributor

I'm inclined to say that if it doesn't move the needle on kraken/sunspider/octane/web-tooling, then we should, ceteris paribus, leave it out (and of those four web-tooling counts the most because it consists of real-world code.)

That said, if you feel strongly that this PR improves the status quo, let me know and I'll review it in more detail.

@littledivy
Copy link
Contributor Author

I'm fine with either, don't feel strongly about it but seems like something that would have be done at some point anyways.

We should probably also remove the comment here:

/* XXX: could generate specific bytecode */
static __exception int js_parse_class_default_ctor(JSParseState *s,

Copy link
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, I think this is in fact a great improvement over the existing code.

@bnoordhuis bnoordhuis merged commit a8a5ecb into quickjs-ng:master Nov 24, 2023
19 checks passed
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.

2 participants