Two - Githubissues

mirisuzanne commented 6 years ago

deep-get tokens with -> syntax (Fixes #12)
use contrast-light/dark for tint/shade (with better defaults and overrides) (Fixes #17)
allow min-OR-max adjustments for fixing query overlap (#24)
tools for managing css variables based on accoutrement maps
"private" tokens with _name syntax (currently only affect css-variable creation)

mirisuzanne commented 5 years ago

@joelschou I released an alpha with all these new features. If you are interested in trying it out: npm install accoutrement@pre

joelschou commented 5 years ago

I gave it a shot this afternoon on my big work project. Once I got the breaking changes fixed (not too much work!), I ran into some pretty intense performance issues. Synopsis:

I pared back my code to isolate the problem. I have this: color: color( 'ui->h1->color' ). Compiling takes 1189ms. If I comment it out, 590ms. If I turn on just five more similar strings (h2 through h6), compiling goes all the way up to 4031ms. Across my entire project, you can imagine the impact that had!

I haven't dug into the parsing of nested items, but I have to assume _a_replace is the culprit. Hrm.

joelschou commented 5 years ago

P.S. I plan to pull down the alpha separately to see if I can build a pure test, just in case I'm doing something wack to trigger this.

joelschou commented 5 years ago

Super quick-n-dirty test case, but should be sufficient to show the problem. https://github.com/joelschou/accoutrement2-test

joelschou commented 5 years ago

I did a little more testing this morning. I moved the -> handling code from my approach into _a_replace and it was every bit as slow as what you were doing. There's just so much [necessary!] looping in replace that adding this additional parsing code grinds it to a halt.

mirisuzanne commented 5 years ago

Yeah, we had that experience when we tried adding nested-maps to Susy several years ago. I hoped modern compilers would make a difference. A few options:

We release the feature, and simply document that -> references will run significantly slower.
We try to make it faster
We ditch it all together

A few theories to try and speed it up:

currently, you can reference nested-siblings and ancestor-siblings without any distinction. Both use the short #hash without any -> needed. That means looking in more places for any given key. It might be faster if we know where to look. Either: a. Assume all references start at the root, and require -> to reference any nested keys b. Require a <- (or similar) syntax to move up the chain and reference ancestor map keys
There might be ways to incorporate some form of map-reference memoization?

$map: (
  'level': 1,
  'nested': (
    'level': 2,
    'from-root': '#level', // 1
    'from-root-nested': '#nested->level', // 2
    'deep': (
      'move-up': '#<-level', // 2
      'move-up-2': '#<-<-level', // 1
    ),
  ),
);

Looking at those in practice, I like the always-from-root approach better than the <- option. I think we could combine that with a step that flattens the map before parsing:

$map: (
  'level': 1,
  'nested->level': 2,
  'nested->from-root': '#level',
  'nested->from-root-nested': '#nested->level',
  // this solution could support the other syntax, but you get the idea…
);

I wonder if that would increase the speed enough?

mirisuzanne commented 5 years ago

@joelschou I took a stab at the changes I mentioned above. I expect nested tokens will still add some compile time still, but hopefully this helps keep that increase from becoming exponential.

Note the two changes from earlier:

BREAKING: Explicit value/adjustments maps require a '%value' key (used to be named 'value'), in order to avoid possible conflicts or false-positives
NEW (BREAKING alpha.1): all nested-map token references (using ->) must be absolute paths from the map root, rather than assuming the most closely related sibling or ancestor value

joelschou commented 5 years ago

Sorry for the slowness. I finally got around to testing this.

Using 3aba49f:

    👉  Starting node-sass...
    ✅  File written to [snip]
    ⏱  Duration: 3881ms

Using 9f11ddb:

    👉  Starting node-sass...
    ✅  File written to [snip]
    ⏱  Duration: 1058ms

That's significant to say the least!

mirisuzanne commented 5 years ago

@joelschou that's great! Is that in a test document, or applied to a project?

@jgerigmeyer we need to finally build that Sass compile-time performance-testing project we've talked about… :)

joelschou commented 5 years ago

That's the project described above, so it's a fairly significant codebase.

mirisuzanne commented 5 years ago

@joelschou great! Is it easy to compare that with a pre-nested compile time?

joelschou commented 5 years ago

I don't have an easy way to compare to the project pre-nested, but I can compare the current master branch with an accoutrement2 branch. The speedup is every bit as significant as my example above!

Edit: numbers:

Using my approach to nesting:

    👉  Starting node-sass...
    ✅  File written to [snip]
    ⏱  Duration: 5484ms

Using 3aba49f:

    👉  Starting node-sass...
    ✅  File written to [snip]
    ⏱  Duration: 4887ms

Using 9f11ddb:

    👉  Starting node-sass...
    ✅  File written to [snip]
    ⏱  Duration: 1434ms

mirisuzanne commented 5 years ago

🎉

joelschou commented 5 years ago

It turns out the speedup wasn't quite as significant as I'd thought. There was a bunch of commented-out code I'd overlooked 😦

It's more like a 25% reduction instead of the 70% I was seeing before.

joelschou commented 5 years ago

Crap. And even more disappointingly, when I revert to my nesting technique it's 3x faster than your improved one.

mirisuzanne commented 5 years ago

@joelschou remind me what your nesting technique was?

joelschou commented 5 years ago

Something a bit more brute-forceish: https://github.com/oddbird/accoutrement/pull/18/files#diff-85139a69ed49378c904ab09d98582c3bR148

It doesn't allow -> in keys (only values) and relies on type checking while parsing instead of the find/replace approach you used.

mirisuzanne commented 5 years ago

ok @joelschou – let me know if you don't have time for my fiddling with various solutions. :)

This latest commit (eaad6fc) sets it to only flatten the map structure when -> is present in the token string.That cut my test-runs from ~2.6s to ~1.9s. When I remove the feature entirely, I only shave off another few milliseconds. Am I missing something? How does this stack up in your code?

joelschou commented 5 years ago

Ran it a few times each. CSS output was, fortunately, identical.

9f11ddb was consistently about 10s. eaad6fc was consistently about ... 3.5s!

oddbird / accoutrement

Two #25