Skip to content

Revert "chore: add some static optimization (#294)"#344

Merged
stephenamar-db merged 1 commit intomasterfrom
stephenamar-db/revertoptimizations
Apr 29, 2025
Merged

Revert "chore: add some static optimization (#294)"#344
stephenamar-db merged 1 commit intomasterfrom
stephenamar-db/revertoptimizations

Conversation

@stephenamar-db
Copy link
Copy Markdown
Collaborator

@stephenamar-db stephenamar-db commented Apr 29, 2025

This partially reverts commit 60db9d0.

This is causing performance regressions

@stephenamar-db stephenamar-db force-pushed the stephenamar-db/revertoptimizations branch from ad4a1b0 to bd88105 Compare April 29, 2025 04:29
@stephenamar-db stephenamar-db force-pushed the stephenamar-db/revertoptimizations branch from bd88105 to 89416b5 Compare April 29, 2025 04:45
@stephenamar-db
Copy link
Copy Markdown
Collaborator Author

@He-Pin it's going to be a partial revert, because your optimizations fixed a bug we had with short-circuiting boolean ops.

@stephenamar-db stephenamar-db merged commit aeb6b9a into master Apr 29, 2025
@stephenamar-db stephenamar-db deleted the stephenamar-db/revertoptimizations branch April 29, 2025 04:48
@He-Pin
Copy link
Copy Markdown
Contributor

He-Pin commented Apr 29, 2025

@stephenamar-db Thank you. I think another weird bug left is #331, anyway, we are unlikely to write that code :)

@stephenamar-db
Copy link
Copy Markdown
Collaborator Author

My gut feeling is that the optimizations around Val.Obj caused them to be evaluated early.

@stephenamar-db
Copy link
Copy Markdown
Collaborator Author

@stephenamar-db Thank you. I think another weird bug left is #331, anyway, we are unlikely to write that code :)

I'm looking at this too. trying to figure what what needs to be changed in the optimizer to make them work.

@He-Pin
Copy link
Copy Markdown
Contributor

He-Pin commented Apr 29, 2025

Cool, I think the bug is if there is an a[x] , where [x] is a params, and then, we should not visit the x which is Id(x) to resolve the parameter, because before evaluation, the x is not binding to the actually value.

That's my guess, and another thing is, we may just pass the Field to the Obj.Comp, because it now contains two cases.
it turn out the slice one is simpler for me to handle :)

case BinaryOp(pos, Val.Num(_, l), BinaryOp.OP_<=, Val.Num(_, r)) => Val.bool(pos, l <= r)
case BinaryOp(pos, Val.Str(_, l), BinaryOp.OP_<=, Val.Str(_, r)) => Val.bool(pos, l <= r)
case BinaryOp(pos, Val.Num(_, l), BinaryOp.OP_>=, Val.Num(_, r)) => Val.bool(pos, l >= r)
case BinaryOp(pos, Val.Str(_, l), BinaryOp.OP_>=, Val.Str(_, r)) => Val.bool(pos, l >= r)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we can extract all BinaryOp to a single method with transformBinaryOp, which may help too.
@stephenamar-db

@He-Pin
Copy link
Copy Markdown
Contributor

He-Pin commented Apr 29, 2025

@stephenamar-db What's the number after revert this.

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