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

Improve memory footprint when evaluating in loops #2171

Merged
merged 1 commit into from
Sep 7, 2016

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Sep 6, 2016

We are cloning/copying too many objects when evaluating loops. i.e. a simple map-get call will always clone the full map that is passed, which can lead to an absurd amount of allocated memory. But we cannot really cache the results as 1) variables can be re-assigned and 2) some code paths (i.e. equality checks) evaluate binary expressions different than normaly. This fix will only cache "delayed" results, which should cover 90% of the cases. This should also improve performance in certain cases by a lot.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 79.203% when pulling da1bc02 on mgreter:improve-mem-footprint into aea48f6 on sass:master.

@mgreter
Copy link
Contributor Author

mgreter commented Sep 6, 2016

This would be ready but is currently blocked by sass/sassc#187 (local MSVC tests pass)

@mgreter mgreter added this to the 3.3.7 milestone Sep 6, 2016
@mgreter mgreter self-assigned this Sep 6, 2016
@mgreter mgreter merged commit 325958f into sass:master Sep 7, 2016
@xzyfer
Copy link
Contributor

xzyfer commented Sep 7, 2016

I have a feel delayed values are not long for this earth

@xzyfer
Copy link
Contributor

xzyfer commented Sep 28, 2016

This broke a lot of eyeglass tests i.e. https://github.com/sass-eyeglass/eyeglass/blob/master/test/test_assets.js#L31-L52

I need to investigate further after some sleep.

This was referenced Sep 28, 2016
@xzyfer xzyfer modified the milestones: 3.3.7, 3.4 Oct 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants