SomMeri / less4j

Less language is an extension of css and less4j compiles it into regular css. Less adds several dynamic features into css: variables, expressions, nested rules, and so on. Less was designed to be compatible with css and any correct css file is also correct less file.
145 stars 47 forks source link

Poor performance due to creating data snapshots #316

Open a701440 opened 8 years ago

a701440 commented 8 years ago

On large less files with a lot of @import(s) and mixins the performance is very poor. It appears to be due to the code constantly running function createCurrentDataSnapshot. If I understand the purpose of this code correctly it is there to preserve the values of variables and mixins in a scope. I think there should be a way to go with some kind of "copy on write" implementation since most scopes don't modify things. In my test files I see upwards of 190 levels with only a few levels having any hashmap(s) in them.

SomMeri commented 8 years ago

You understand it right. It allows easy discard of changes made by mixins and to save the state of the scope for mixins, so that they use variables as they were at the time they were defined (even if imported etc).

It should be easy to create the lazy clone. It would need to be created on write and probably also whenever toIndependentWorkingCopyAllParents is created - due to mixins.

a701440 commented 8 years ago

I have experimented with the following changes to LocalScope class:

private LocalScopeData localData = new LocalScopeData(); private Stack localDataSnapshots = new Stack(); private boolean localDataCloned; private boolean localDataCloneOnWrite;

public void createCurrentDataSnapshot() { localDataSnapshots.push(localData); this.localDataCloneOnWrite = true; this.localDataCloned = false; } private void cloneLocalData() { if (this.localDataCloneOnWrite && !this.localDataCloned) { localData = localData.clone(); this.localDataCloned = true; } }

public void discardLastDataSnapshot() { localData = localDataSnapshots.pop(); this.localDataCloneOnWrite = false; this.localDataCloned = false; }

Then all methods that modify the LocalScope add "cloneLocalData" call like this:

public void registerMixin(ReusableStructure mixin, IScope mixinsBodyScope) { cloneLocalData(); getLocalMixins().store(new FullMixinDefinition(mixin, mixinsBodyScope)); }

This reduced the number of "clone" calls by a factor of 2 in your unit tests. I think it's somewhat sub-optimal because I had to call clone in more cases then absolutely necessary, but it helps any way.

SomMeri commented 8 years ago

Related to #320

SomMeri commented 8 years ago

The performance pack pull request implements "copy on read" instead of "copy on write" strategy. Is that enough to close this issue or should I leave this open?