Upgrading patternlab to v1.3.0 is doubling my compilation speed

[bramsmulders]

I am using Pattern Lab Node v1.3.0 on Mac, using the grunt configuration.

Quick note

I use pl node for over 9 months now on a very big project. I have many patterns and lots of data so builds were taking ~12 seconds on each save. We are in the progress of updating/refactoring the styleguides to make it smaller and leaner.

Expected Behavior

Build times to take ~12 seconds.

Actual Behavior

With upgrading to pl-node v1.3.0 compiling takes around ~24 seconds. This is twice as long than it took before.

Steps to Reproduce

Switch between v1.2.2 and v1.3.0, and see the differences

[e2tha-e]

@bramsmulders Out of curiosity, could you check out the latest commit from pr #277 , run a build, and check the time? This would be commit 470f54d from https://github.com/e2tha-e/patternlab-node.git , branch 250-with-recursion-limiter

[bmuenzenmeyer]

As @e2tha-e points out - there is some recursion that made it into this version to fix an issue, but sans the perf improvements e2th-a is addressing in that PR. Unfortunately, that PR is too big to merge in its current state and is likely destined for dev-2.0-core. I take full responsibility for pushing the version a bit half-baked and with potential performance problems. For the meantime, I've updated the release with a note regard potential issues.

[bramsmulders]

@e2tha-e That branch is working as it should, compiling takes ~12 seconds again. So i can confirm this recursion limiter works.

@bmuenzenmeyer can you cherry-pick the commit which entails the perf improvements?

[e2tha-e]

@bramsmulders Super glad to hear! Thanks for taking the time to test!

[bmuenzenmeyer]

@bramsmulders I am familiar with that concept but have never tried it - especially against such a large commit set.

I am thinking now that an acceptable approach may be to review for acceptance #277 on the 1.X.X track (as it already passes tests and is merge-able) and then work to forward port it (perhaps via a cherry-pick) into dev-2.0-core, which is also engine aware.

Looking for feedback from you all, including @e2tha-e and @geoffp on that.

[e2tha-e]

@bramsmulders @bmuenzenmeyer
Just as an FIY, Git cherry picking from pr #277 will be impossible given the scale of the pr and the number of commits, rebases, etc. You'll really have to consider how you terminate the development flow of the 1.x.x major version, and how you'll resume development on the 2.x.x major version. Given that in semver, major versions are not backwards compatible, it should be fine to just merge the pr into 1.x.x, and redo the pr for 2.x.x. I am totally fine with this option. However, this is just a suggestion, and I leave the room open for discussion and debate.

[bmuenzenmeyer]

@e2tha-e
I'd like to think through it a bit more, but your proposal aligns with my thinking - assuming you are okay with porting this functionality over to 2.x.x. as your schedule allows - it's a big ask!

[geoffp]

I'm still nervous, but from scanning it as well as I can, it looks good, and as long as @e2tha-e is committed to forward porting it to dev-2.0, I'll sign off!

When it does get forward ported, I'd expect most of this to find its way into the Mustache engine. @e2tha-e, for bonus points, move the pattern parameter and stylemodifier machinery into the Mustache engine. :) Nah, I kid, that's totally a separate PR.

[e2tha-e]

@bmuenzenmeyer @geoffp Yes, I'm looking forward to getting this into dev-2.0!

[e2tha-e]

The work for this is in the pull requests referencing issue #250

[bramsmulders]

Hello again, i was updating from PL node 2.5.1(where this issue wasnt present anymore) to v2.6.1 in a new project(average build times: ~6 seconds). And the build times increased from ~6 seconds to ~12 seconds.

Is there something pushed which breaks the fix mentioned here?

[bmuenzenmeyer]

Incremental builds are the solution for this going forward. I am interested in more performance improvements if they are small and aimed at memory management and the like.

[bramsmulders]

This is actually not fixed by incremental builds. The problems from this bug caused the total bouild time to be doubled. Incremental builds only shave off half of that(already doubled up) time. So no actual gains here.

The problem lies within the use of lots of data. For every separate data.json file the build takes longer to load even before actually parse templates.

[tburny]

I am reopening this issue because @bramsmulders is correct here.

[bramsmulders]

I'm going to create a demo data/pattern set to demonstrate it tonight.

[tburny]

That would be amazing! Maybe we can do some profiling then?

[bramsmulders]

hmmm, i was upgrading a project of ours to 2.7.2. Lots of templates, lots of data, after upgrading and running patternlab i get an JavaScript heap out of memory error. This happens after 2 minutes of compiling. This project was coming from v1.2.2 where builds usually took like ~10 seconds.

In comparison:
The same set of data & patterns compiles easily within 2.5 seconds in PL-PHP.

I cannot grasp this amount of difference between PHP & node, i can live with built times up to ~10 seconds but this is affecting the workflow a LOT more.

I will recreate the amount of data with no company related stuff inside to test.

[tburny]

By the way what pattern engine do you use?

Can you try do rebuild your project with cleanOutput = true? This should disable Incremental Builds so we can exclude it as a cause of the problems.

Also maybe you can try get a heap dump?

[bramsmulders]

I'm using the mustache engine, out of the box.
With incremental builds disabled everything compiles, but still takes ~127 seconds per build.

All templates are processed in the console in 90 seconds but the task is not completed then, it waits for an additional 30 seconds to complete the task

[geoffp]

I did some profiling for the asynchronous building work that found that a major source of delay is the HTML beautification step. I forget what the option is called just now (on my phone).

[bmuenzenmeyer]

I forget what the option is called just now

cleanOutputHtml

[geoffp]

That's the one. I had the impulse to implement a worker thread pool to make it faster, but I controlled myself. @bramsmulders, do you have cleanOutputHtml set?

[bramsmulders]

Will check in a few, hold on

[bmuenzenmeyer]

@bramsmulders can you point to dev and retest please?

[bmuenzenmeyer]

I think I have fixed this with PL Node 2.9.X

[bramsmulders]

@bmuenzenmeyer Oh yeah! first tests are promising! From ~12 to ~4 seconds, cold start.
Incremental builds now take ~1.3 instead of ~4

[bmuenzenmeyer]

@bramsmulders excellent news. Makes sense really, since I basically reverted the logic that caused the problem in the first place.

Are you alright with closing this issue, at long last ? 😄

[bramsmulders]

Just do it! 💪🏻

i'll do it 😉

Thnx for all the hard work on making this a success!

[bmuenzenmeyer]

👍

sorry it took so long to identify the culprit. thanks for sticking with the tool and living with slower performance for 11 months!