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

Optimize integer operations #1147

Closed
RageKnify opened this issue Feb 28, 2021 · 6 comments
Closed

Optimize integer operations #1147

RageKnify opened this issue Feb 28, 2021 · 6 comments
Assignees
Labels
discussion Issues needing more discussion enhancement New feature or request execution Issues or PRs related to code execution performance Performance related changes and issues

Comments

@RageKnify
Copy link
Contributor

Increment operators on Value::Integer(i32)s result in Value::Rational(f64)s because they use Value::to_number which returns f64. This makes traditional for loops with i++ slower because they use floating point arithmetic rather than integer arithmetic.


My idea

I thought a good solution would be to have an internal:

enum Number {
  float(f64),
  int(i32)
}

We would also simplify to have a single Value variant for number rather than the current duo of Value::Rational and Value::Integer.
This would be the new return type of Value::to_number using the int variant when possible. It would also probably be a good fit tor the Number variant of ObjectData.

Open to other ideas 💡

@RageKnify RageKnify added enhancement New feature or request performance Performance related changes and issues discussion Issues needing more discussion execution Issues or PRs related to code execution labels Feb 28, 2021
@HalidOdat HalidOdat self-assigned this May 12, 2021
@Razican
Copy link
Member

Razican commented Jan 31, 2022

@HalidOdat I see you assigned yourself this issue, let me know if you're still working on it, or if we can un-assign it :)

@HalidOdat
Copy link
Member

@HalidOdat I see you assigned yourself this issue, let me know if you're still working on it, or if we can un-assign it :)

I didn't implement this because it would make #1373 harder/impossible since integer and rational has will be encoded JsValue into a 8-byte value and we can't fir an enum Number (16-byte). It would be added just to be removed when NaN boxing gets added.

@Razican
Copy link
Member

Razican commented Feb 3, 2022

@HalidOdat I see you assigned yourself this issue, let me know if you're still working on it, or if we can un-assign it :)

I didn't implement this because it would make #1373 harder/impossible since integer and rational has will be encoded JsValue into a 8-byte value and we can't fir an enum Number (16-byte). It would be added just to be removed when NaN boxing gets added.

Still, having integer operations optimized would be a good thing, right? Do you see a way we could get this? An option might be to use untagged enums (unions) or somehow add different JsValue variants.

@HalidOdat
Copy link
Member

Still, having integer operations optimized would be a good thing, right? Do you see a way we could get this? An option might be to use untagged enums (unions) or somehow add different JsValue variants.

We are still going to have integer and rational variants, we wont remove them! But they have to be flat (since the tag is encoded with the payload, all in a 64bit value), we can't nest it (the current implementation is this way anyway).
In terms of optimization it would be better to make to_number() return the enum Number, since it turns integer to rational (f64). In any case the benefits we get from nan-boxing out weight the benefit of abstraction from Number enum, since we can store the JsValues in 64 bit registers and we use less memory which also results in better memory caching.

@RageKnify
Copy link
Contributor Author

As said in #1964, while it makes some work in this area I still believe more could be done inside boa_engine, I might be wrong, but I believe using integers instead of floats when possible would be a worthwhile optimization since integer operations are faster.
Though we should definitely do some proper bench-marking before merging this kind of change.

@jedel1043
Copy link
Member

We can close this from the fact that we already have optimizations to preserve integers on general operations like sums, subtractions, mod, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Issues needing more discussion enhancement New feature or request execution Issues or PRs related to code execution performance Performance related changes and issues
Projects
None yet
Development

No branches or pull requests

4 participants