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

RD-10562 Implement OSR for iterators #352

Closed
wants to merge 40 commits into from

Conversation

alexzerntev
Copy link
Contributor

@alexzerntev alexzerntev commented Feb 4, 2024

  • Updated aggregations to use OSR
  • Updated lists to use OSR
  • Updated joins to use OSR
  • Updated iterables to use OSR
  • Updated Json list reader to use OSR
  • RecordObject now extends Dynamic Truffle object

@alexzerntev alexzerntev marked this pull request as ready for review February 5, 2024 21:04
Copy link
Collaborator

@bgaidioz bgaidioz left a comment

Choose a reason for hiding this comment

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

A question about the design. It felt to me like the implementation doesn't use the RepeatingNode correctly. But I only found a specific example to compare this to.

return returnValue == this.initialLoopStatus() || currentIdx < llist.size();
}

public Object executeRepeatingWithValue(VirtualFrame frame) {
Copy link
Collaborator

@bgaidioz bgaidioz Feb 8, 2024

Choose a reason for hiding this comment

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

The doc of executeRepeatingWithValue says: "return a value v satisfying shouldContinue(v) == true if the method should be executed again to complete the loop and any other value if it must not." (That last part I interpret as "any other value that doesn' t satisfy shouldContinue.)

I see shouldContinue's code is comparing its parameter to initialLoopStatus, which is a status, an object like CONTINUE_LOOP_STATUS. But with executeRepeatingWithValue returning the result array, it will perform the same test with that array. I understand that will evaluate to false and only the check on currentIdx is going to matter. But that feels like not the intention, to spend the whole loop execution comparing the array to the "enum".

I interpret the OptimizedOSRLoopNode logic as if the return value of the node is a status value (and can be custom if using the withValue version: BREAK, TOOK_TOO_LONG, INTERRUPTED, etc.) and the way a loop would return an actual result would be either as a side-effect (by updating a mutable object passed as a parameter) or by exposing a getResult() method that returns an object it would have allocated internally. e.g. all these OSR...Node classes would compute the result internally, and inherit an abstract class (which itself inherits RepeatingNode), that would require a getResult() method. Which is called after execute?

Node existNode = new ExistNode();
existNode.execute(); // returns the status, in our case `true/false`, no need for a custom status?
return existNode.getResult();

I looked in TruffleScheme code a bit everywhere to find how loop-nodes that return a value are implemented. Scheme has plenty, like length, map, etc. All the expected ones in fact work with regular for loops instead of using RepeatingNode (they probably didn't fix them yet..?). There's one place I found the TailRecursiveLoop (which is a RepeatingNode) called, and it goes with calling execute (ignoring its result) and picking the result from elsewhere. In that specific case, the node returning a value through execute (TailRecursiveRootNode) is initialized with an index where to write the result too, and the looping node (TailRecursiveLoopNode) is getting it as a parameter. Internally, it writes in that index, while looping with a regular status implementation. Which is quite like it would be initialized with a mutable object and update it as the loop runs.

@Override
public Object executeGeneric(VirtualFrame frame) {
Object generator = frame.getAuxiliarySlot(generatorSlot);
if (!hasNextNode.execute(this, generator)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

todo, remove the if

@alexzerntev
Copy link
Contributor Author

alexzerntev commented Mar 27, 2024

To be closed, will be merged through #379

@alexzerntev alexzerntev deleted the RD-10562-implement-osr-for-iterators branch March 29, 2024 13:24
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