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

Closure tracking #2357

Merged
merged 7 commits into from Nov 11, 2022
Merged

Closure tracking #2357

merged 7 commits into from Nov 11, 2022

Conversation

hmdne
Copy link
Member

@hmdne hmdne commented Nov 9, 2021

The closures make everything harder for how Opal is right now.

Consider this theoretical code:

proc { a = (true and next); 6 }

What does this proc return? 6. Why? Because a closure stands in a way and next is ONLY compiled to return. A lot more issues - albeit you really need to try to break it - but some existing code may show deficiencies, eg. when you get two returners entangled.

So I propose to track the closures and use this knowledge to correctly dispatch the breakers/nexters/returners/retriers. If we get a closure in the way - we need to use exceptions.

Commit description of the finalized patch:

    This adds closure awareness to Opal. Previously, some control flow
    instructions were working based on the most common situation and
    in certain situations we got that wrong. The previous implementation
    also led to a lot of scattered around code and a lot of logic
    duplication. My view is that this patch improves maintainability
    of our codebase a lot and allows for further improvements based on
    the new mini-DSL. Unfortunately, one of the downsides of this code
    is that runtime performance is slightly decreased.

Also, I have verified that all linked issues are fixed by this patch.

@hmdne
Copy link
Member Author

hmdne commented Oct 29, 2022

This now implements both break and next correctly. It is still too early to merge and performance got quite a heavy hit. But to ensure everything works correctly, this patch still needs some time.

@hmdne hmdne added this to the v1.7 milestone Oct 31, 2022
@hmdne
Copy link
Member Author

hmdne commented Nov 2, 2022

The question here - I may have that patch finished until next week. Do we get this for 1.6, or do we postpone it to "1.7" whatever the next release may be.

This adds closure awareness to Opal. Previously, some control flow
instructions were working based on the most common situation and
in certain situations we got that wrong. The previous implementation
also led to a lot of scattered around code and a lot of logic
duplication. My view is that this patch improves maintainability
of our codebase a lot and allows for further improvements based on
the new mini-DSL. Unfortunately, one of the downsides of this code
is that runtime performance is slightly decreased.
@hmdne hmdne modified the milestones: v1.7, v1.6 Nov 5, 2022
@hmdne hmdne marked this pull request as ready for review November 5, 2022 21:47
@hmdne hmdne changed the title [wip] Closure tracking Closure tracking Nov 5, 2022
@hmdne
Copy link
Member Author

hmdne commented Nov 5, 2022

A problem found with Opal-RSpec working branch:

[user@localhost opal]# bin/opal -e 'def x; lambda do; return 3; end; end; p x.()'
[Opal]: Zlib::BufError; retrying
<internal:corelib/runtime.js>:2969:5:in `x': unexpected return (Exception)
	from -e:1:1:in `x'
	from -e:1:41:in `undefined'
	from <internal:corelib/runtime.js>:2805:7:in `<main>'
	from -e:1:1:in `null'
	from node:internal/modules/cjs/loader:1105:14:in `Module._compile'
	from node:internal/modules/cjs/loader:1159:10:in `Module._extensions..js'
	from node:internal/modules/cjs/loader:981:32:in `Module.load'
	from node:internal/modules/cjs/loader:822:12:in `Module._load'
	from node:internal/modules/run_main:77:12:in `executeUserEntryPoint'
	from node:internal/main/run_main_module:17:47:in `undefined'
[user@localhost opal]# ruby -e 'def x; lambda do; return 3; end; end; p x.()'
3
[user@localhost opal]# 

@@ -10,6 +10,9 @@ class EnsureNode < Base
children :begn, :ensr

def compile
# FIXME: We don't need this if there's no rescue else
push_closure
Copy link
Member

Choose a reason for hiding this comment

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

push_closure if has_rescue_else?

maybe

Copy link
Member Author

Choose a reason for hiding this comment

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

unfortunately no... has_rescue_else? can be called only after we have traversed thru all the child nodes. as a followup to this, I plan to mark the throwers using a rewriter, replacing the current measures and the BreakFinder rewriter

@@ -49,6 +52,8 @@ def compile

line '}'

pop_closure
Copy link
Member

Choose a reason for hiding this comment

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

pop_closure if has_rescue_else?

maybe?

BreakFinder used to be called on-demand, now ThrowerFinder is a
regular rewriter, implementing an early pass of closure tracking.

This should provide some small reduction of generated code.
Don't create an IIFE, if our catcher is a JS_FUNCTION, because
it means, that immediately after compiling a catcher we will
wrap things with some kind of a function, so semantics will
remain.
This is similar to what it used to be called before and also we can
spare a couple of bytes.
@hmdne
Copy link
Member Author

hmdne commented Nov 10, 2022

  • toplevel return breaks with this patchset

Copy link
Member

@elia elia left a comment

Choose a reason for hiding this comment

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

As always great to see all those specs go away 🎉

@elia elia merged commit 2dce8c3 into master Nov 11, 2022
@elia elia deleted the hmdne/closure-tracking branch November 11, 2022 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants