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

[Truffle] Discussion: introducing first-class Array strategies #3784

Closed
wants to merge 1 commit into from

Conversation

eregon
Copy link
Member

@eregon eregon commented Apr 7, 2016

@chrisseaton @nirvdrum @pitr-ch I am thinking to use ArrayStrategy objects for our strategies on Array to reduces the number of specialization and duplication by 4.
This is only a preview to get some feedback (do not merge).
What do you think?
Is it nice / too complex / not that helpful?

This is maybe slightly overlapping with ArrayMirror, although ArrayMirror is really about helpers for Java arrays, and this is really about the storage strategies for our Ruby Arrays.
In the current version, an ArrayStrategy can build an ArrayMirror easily so there is no conflict of features. There are only 5 ArrayStrategy which are singleton instances, so this also avoids potential allocation in the interpreter.

An alternative solution would be to have one node per "functionality" of Array, like "get at index", "set at index", "grow", "delete/shrink", etc. This already exists to some extent but it's underused because it's quite verbose, there would be many many features like that for Ruby Array and it's not obvious to identify them either while still keeping the performance.

@eregon eregon added the truffle label Apr 7, 2016
@eregon eregon added this to the truffle-dev milestone Apr 7, 2016
* Reduces the number of specializations and duplication by 4.
@eregon eregon force-pushed the array_strategies branch from 2b9f1e1 to db55141 Compare April 7, 2016 16:57
@nirvdrum
Copy link
Contributor

nirvdrum commented Apr 7, 2016

Upon first reading, I like this overall. It's hard to trace the specializations given how many of them there are. However, I find pushing more logic into guards and @cached expressions also makes things hard to reason about. The execution order there is a bit backwards and finding where the static method is a bit annoying (no IDE help without going to the generated classes).

The other part I like about this is it opens the door for more strategies without having to fight with even more specialization explosion.

@pitr-ch
Copy link
Member

pitr-ch commented Apr 7, 2016

I like it! It clearly defines the strategies and its interface.

@chrisseaton
Copy link
Contributor

The reason I didn't originally do it this way was because I thought I could save an object reference - if we had an int[] I would know the strategy to use, and it seemed cool that the strategies would just be specialisations with guards. I think PE also did not work the same back then - you needed a VirtualFrame parameter for stuff to be inlined.

I think it's right to move beyond that to some specialisation classes.

If you are experimenting, try some more unusual strategies early to catch if you are painting yourself into any corners. For example maybe a Bitset specialisation for boolean arrays? Or some kind of spare array?

You've probably already read Laurie Tratt's et al's strategies paper, but if you haven't do that.

@eregon
Copy link
Member Author

eregon commented Apr 8, 2016

The reason I didn't originally do it this way was because I thought I could save an object reference - if we had an int[] I would know the strategy to use, and it seemed cool that the strategies would just be specialisations with guards.

It's still the case, the array layout is unchanged, it's just a way to express code more generically.

@eregon
Copy link
Member Author

eregon commented Apr 10, 2016

Thanks for the feedback!
I'll integrate this next week.

@eregon eregon closed this Apr 10, 2016
@eregon eregon deleted the array_strategies branch April 10, 2016 10:54
@enebo enebo added this to the Non-Release milestone Dec 7, 2017
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.

None yet

5 participants