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

Avoid block splitting when not necessary #177

Merged
merged 3 commits into from
Jul 29, 2017
Merged

Avoid block splitting when not necessary #177

merged 3 commits into from
Jul 29, 2017

Conversation

smarr
Copy link
Owner

@smarr smarr commented Jul 27, 2017

Blocks were split unconditionally, even when there was no obvious benefit or need to split them.
This can lead to excessive code duplication, especially since Truffle splitting applies to block methods in addition to the lexical splitting.

A few more details are documented here: http://markmail.org/thread/dg42ud652xr5rupf

Generally, splitting is needed to make sure that lexically nested blocks access the correct frame slots when they access outer variables.
Since this is only critical to get right if we merged an outer scope, or when there are actually variable accesses, we can avoid the splitting in all other cases.

@Richard-Roberts, this is a special gift to you. Yet another condition you'll have to consider for object literals 😁. Thought it might be too easy, or you get bored with them 😜

Please review.

@smarr smarr added bug Fixes an issue, incorrect implementation enhancement Improves the implementation with something noteworthy labels Jul 27, 2017
@smarr smarr added this to the v0.5.0 milestone Jul 27, 2017
@smarr smarr self-assigned this Jul 27, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 80.208% when pulling 918c3b2 on avoid-block-splitting into 4c9480d on dev.

Signed-off-by: Stefan Marr <git@stefan-marr.de>
If scopes are merged, we need to adjust everything.

We also need to adjust everything for split atomic methods.

Signed-off-by: Stefan Marr <git@stefan-marr.de>
Only locals are relevant for splitting decisions, access to arguments is always safe.

Signed-off-by: Stefan Marr <git@stefan-marr.de>
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 79.961% when pulling 56ee0bb on avoid-block-splitting into 1926fe3 on dev.

@smarr smarr merged commit bd532ac into dev Jul 29, 2017
@smarr smarr deleted the avoid-block-splitting branch July 29, 2017 13:07
@smarr
Copy link
Owner Author

smarr commented Mar 7, 2018

This change/optimization turns out to be problematic, because we then don't have a correct scope chain.
An outer scope might have been split, while an inner has not been.
In this case, the inner scope would point to the old outer scope, which means we perhaps don't have to correct frame slot/frame descriptor in cases where we started traversing from an unsplit inner scope.

I am not yet sure how to fix this in a way the minimizes splitting, and avoids unnecessary splitting.

@ghost
Copy link

ghost commented Mar 7, 2018

In what case would it be unnecessary to not split the block? Only when the block does not rely on any context, right?

@smarr
Copy link
Owner Author

smarr commented Mar 7, 2018

since we only have a single parent, it seems like we always need to split if the outer scope changed, otherwise we get a wrong parent/outer scope.

@ghost
Copy link

ghost commented Mar 7, 2018

I'm not clear what you mean by parent sorry, but I'm assuming you mean the context the block is in. So, there are no unnecessary cases then? If so, we should probably revert this.

@smarr
Copy link
Owner Author

smarr commented Mar 7, 2018

parent -> outer scope, i.e. the enclosing scope.

and it is called a parent, because it's a tree of scopes, and we usually refer to the root of a subtree as a parent

@smarr
Copy link
Owner Author

smarr commented Mar 7, 2018

Just reverting this is not really an option. This 'optimization' was put in for a reason. I am currently playing with the idea of not splitting scope at all. Don't know what the performance impact of that is though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes an issue, incorrect implementation enhancement Improves the implementation with something noteworthy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants