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

Fixes to prepare generate-bytecode.js for ts-check #452

Merged
merged 3 commits into from
Dec 11, 2023

Conversation

markw65
Copy link

@markw65 markw65 commented Dec 11, 2023

I started to add jsdoc type annotations to generate-bytecode.js, and found a number of issues that typescript doesn't like. It seemed to make sense to fix those separately. But I can add the actual ts-check pull request (not yet submitted) to this one if you'd prefer.

  1. Using thing | 0 to produce an integer when thing could be undefined

    • this can be useful in some cases - eg if thing might be boolean, or a non-integer, or an integer outside the range of a 32 bit number. But in every case, it's a match field which is always one of -1, 0, 1 or undefined. So thing || 0 does the same thing, and typescript is happy with it.
  2. Putting null into arrays that are supposed to contain numbers

    • In every case it was to avoid adding something to one of the literal arrays, when the match field says that it won't be used. So using -1 is just as good.
  3. Using Object.entries with lib set to ES2015

    • If you think ES2017 is universal enough, we could just bump the lib version. With this diff I've gone with switching to Object.keys.

@hildjj
Copy link
Contributor

hildjj commented Dec 11, 2023

  1. Last time that we discussed |0, I recall @Mingun having opinions. Typescript's reaction might change their mind.
  2. nod
  3. I think switching to Object.keys is probably better for now, even though it's been 'broken' for long enough that we can probably just switch lib version. I'd like to have a larger discussion about version support after we refactor code generation.

@Mingun
Copy link
Member

Mingun commented Dec 11, 2023

|0 is used only as idiomatic way to convert everything into an integer in JS. In theory, that also should help JIT that will see that the property variable has only integer shape. If something changed since those times, you can refactor code to reflect that.

@hildjj
Copy link
Contributor

hildjj commented Dec 11, 2023

While I agree that |0 is probably still better, a) Typescript doesn't understand it and b) I don't think this is going to make enough performance difference to matter, so let's move forward.

@markw65 do you want me to merge this now, or would you like to add another commit that turns on ts-check first? (regardless, will need a changelog)

@markw65
Copy link
Author

markw65 commented Dec 11, 2023

Let's just go with this. I'll send a ts-check pull request shortly.

Btw, @Mingun is right that its faster:

node -p 'const start=Date.now(); let x = 0; for (let i = 0; i < 1000000000; i += (x || 0) + 1) {}; Date.now() - start'
1402
node -p 'const start=Date.now(); let x = 0; for (let i = 0; i < 1000000000; i += (x | 0) + 1) {}; Date.now() - start'
698

Similar results if you start with let x = undefined.

But at an extra 700ms for a billion iterations, I don't think it would be possible to measure the impact on peggy...

@hildjj hildjj merged commit 63b913d into peggyjs:main Dec 11, 2023
9 checks passed
MarcelBolten added a commit to MarcelBolten/phpeggy that referenced this pull request Dec 11, 2023
@markw65 markw65 deleted the ts-check-fixes branch December 11, 2023 22:29
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.

3 participants