Skip to content

Fix pokeSTArray, some optimizations #32

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

Merged
merged 2 commits into from
Mar 24, 2015

Conversation

jacereda
Copy link
Contributor

It seems pokeSTArray allows poking one element past the end.

while(++i < n) {
var n = arr.length >>> 0;
var as = new Array(n);
for (var i = 0 >>> 0; i < n; i++)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is 0 >>> 0 for here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to ensure unsigned values. Probably useless in this case (not in the case of peekSTArray, where it saves a comparison). Do you want me to remove this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why would length be signed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just trying to hint the JIT that both values are unsigned:

(-1) < ([].length)
true
(-1>>>0) < ([].length >>> 0)
false


I guess this is completely useless since JITs are now quite smart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We really need a good benchmarking library, I'm trying to wrap benchmark.js right now.

Copy link
Member

Choose a reason for hiding this comment

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

asmjs style? I know x|0 is recognised as "it's an int" type hint, but not sure if there is one for uints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like >>>0 is indeed the hint for unsigned.

Take a look at the first benchmark in http://mbebenita.github.io/LLJS/

Change the loop limit to something like 0x7fffffff in all 3 tests, it seems int is the fastest type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've seen very disparate results across browsers/nodes. I'll remove the hints.

@paf31
Copy link
Contributor

paf31 commented Mar 24, 2015

Looks good to me, thanks. Anyone else want to comment?

@garyb
Copy link
Member

garyb commented Mar 24, 2015

I don't think it's very likely at all, but isn't i >>> 0 problematic here? What if you really did have an array that had 4294967295 elements and tried to use -1 as the index. The original ~~i wasn't much better for similar reasons, and I think that may have been my fault.

Perhaps this is an absurd thing to worry about though.

@jacereda
Copy link
Contributor Author

Should I leave that as normal numbers with 2 comparisons? The important thing here is really the preallocation and the wrong check...

@garyb
Copy link
Member

garyb commented Mar 24, 2015

It's not something I feel strongly about, just thought it was worth discussing. I'd be happy to merge as it is really. @paf31?

paf31 added a commit that referenced this pull request Mar 24, 2015
Fix pokeSTArray, some optimizations
@paf31 paf31 merged commit b376ae8 into purescript:master Mar 24, 2015
@paf31
Copy link
Contributor

paf31 commented Mar 24, 2015

👍

@garyb
Copy link
Member

garyb commented Mar 24, 2015

I might try and make an array that large to see how long it even takes to iterate over it...

@paf31
Copy link
Contributor

paf31 commented Mar 24, 2015

:)

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