refactor(postcss-colormin): Improve caching. #771

ben-eb · 2019-05-23T14:24:29Z

Previously a cache was kept per property/value pair as well as the transformed value. This meant that otherwise identical values would result in a cache miss. Instead, we now cache at the value level and at the colour transform level, which should improve performance.

This patch is also experimenting with a more declarative style using Ramda in order to do the caching, and some small quality of life fixes.

@evilebottnawi What do you think? Should we continue with this style?

codecov-io · 2019-05-23T14:43:02Z

Codecov Report

Merging #771 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #771      +/-   ##
==========================================
+ Coverage   98.69%   98.72%   +0.02%     
==========================================
  Files         110      116       +6     
  Lines        3061     3053       -8     
==========================================
- Hits         3021     3014       -7     
+ Misses         40       39       -1

Impacted Files	Coverage Δ
packages/postcss-colormin/src/lib/cacheFn.js	`100% <100%> (ø)`
packages/postcss-colormin/src/lib/shorter.js	`100% <100%> (ø)`
...ges/postcss-colormin/src/lib/isMathFunctionNode.js	`100% <100%> (ø)`
packages/postcss-colormin/src/index.js	`100% <100%> (+2.17%)`	⬆️
packages/postcss-colormin/src/colours.js	`100% <100%> (ø)`	⬆️
...ackages/postcss-colormin/src/lib/isFunctionNode.js	`100% <100%> (ø)`
packages/postcss-colormin/src/lib/includedIn.js	`100% <100%> (ø)`
packages/postcss-colormin/src/lib/isWordNode.js	`100% <100%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9bc061...27554fb. Read the comment docs.

Previously a cache was kept per property/value pair as well as the transformed value. This meant that otherwise identical values would result in a cache miss. Instead, we now cache at the value level and at the colour transform level, which should improve performance. This patch is also experimenting with a more declarative style using Ramda in order to do the caching, and some small quality of life fixes.

…conversion fix #1042 feat(postcss-colormin): change to colord * round color values fix #819 * reduce dependencies perf(postcss-colormin): improve caching Fix #771

fix(postcss-colormin): do not replace original value if shorter than conversion fix #1042 Also improve the minification in other cases, for example now emits hsla(0,0%,4%,1) instead of rgba(10,10,10,.1) feat(postcss-colormin): change to colord * round color values fix #819 * reduce dependencies perf(postcss-colormin): improve caching Fix #771 refactor(postcss-colormin): drop the generate script Finding the color name is hash lookup in any case. Cannot see how the extra work of loading a JSON file, parsing it, can improve performance.

* rework the colomin cache so use only one cache as in #771 (without using Ramda as in that PR). The cache key is now just the declaration value. Before there were two caches, one with a CSS `property|value` key and one that cached the single colors. * change the underlying library to [colord](https://github.com/omgovich/colord). Using `colord` comes with some trade-offs. On the plus side, it automatically rounds up up, which fixes an [open issue](#819) without additional work, reduces transitive dependencies from 4 to 0 (although I think the previous 4 are all maintained by the same people), is smaller and claims to be faster (haven't checked but I suspect css-value-parser is more the bottle neck than color conversion here). On the minus side, the `colord` parser is very lenient so it accepts invalid CSS colors (rgb with mixed percentages and numbers) which cssnano did not convert before, so I've added some checks to skip these values. Overall there are less lines of code. * get rid of the JSON file with precomputed CSS color names. I guess the idea was that the JSON file only contained color names that are shorter than the hex, but the color conversion libraries include all color names anyway to be able to parse, so in fact we ship the color names twice. I don't think the extra code for generating the file, plus loading the JSON is worth it to avoid a an extra length check. fix(postcss-colormin): do not replace original value with longer one fix #1042 feat(postcss-colormin): switch to colord * round color values fix #819 * reduce dependencies perf(postcss-colormin): improve caching Fix #771 refactor(postcss-colormin): drop the generate script

* rework the colomin cache so use only one cache as in #771 (without using Ramda as in that PR). The cache key is now just the declaration value. Before there were two caches, one with a CSS `property|value` key and one that cached the single colors. * change the underlying library to [colord](https://github.com/omgovich/colord). Using `colord` comes with some trade-offs. On the plus side, it automatically rounds up up, which fixes an [open issue](#819) without additional work, reduces transitive dependencies from 5 to 0 (although I think the previous 4 are all maintained by the same people), is smaller and claims to be faster (haven't checked but I suspect css-value-parser is more the bottle neck than color conversion here). On the minus side, the `colord` parser is very lenient so it accepts invalid CSS colors (rgb with mixed percentages and numbers) which cssnano did not convert before, so I've added some checks to skip these values. Overall there are less lines of code. * get rid of the JSON file with precomputed CSS color names. I guess the idea was that the JSON file only contained color names that are shorter than the hex, but the color conversion libraries include all color names anyway to be able to parse, so in fact we ship the color names twice. I don't think the extra code for generating the file, plus loading the JSON is worth it to avoid a an extra length check. fix(postcss-colormin): do not replace original value with longer one fix #1042 feat(postcss-colormin): switch to colord * round color values fix #819 * reduce dependencies perf(postcss-colormin): improve caching Fix #771 refactor(postcss-colormin): drop the generate script

* ensure that the replacement color values are shorter than the origina. Previously, the plugin returned the converted value without checking whether it was shorter than the original, we now ensure we always return the shortest color value * rework the colomin cache so use only one cache as in #771 (without using Ramda as in that PR). The cache key is now just the declaration value. Before there were two caches, one with a CSS `property|value` key and one that cached the single colors. * change the underlying library to [colord](https://github.com/omgovich/colord). Using `colord` comes with some trade-offs. On the plus side, it automatically rounds up up, which fixes an [open issue](#819) without additional work, reduces transitive dependencies from 5 to 0 (although I think the previous 4 are all maintained by the same people), is smaller and claims to be faster (haven't checked but I suspect css-value-parser is more the bottle neck than color conversion here). On the minus side, the `colord` parser is very lenient so it accepts invalid CSS colors (rgb with mixed percentages and numbers) which cssnano did not convert before, so I've added some checks to skip these values. Overall there are less lines of code. * get rid of the JSON file with precomputed CSS color names. I guess the idea was that the JSON file only contained color names that are shorter than the hex, but the color conversion libraries include all color names anyway to be able to parse, so in fact we ship the color names twice. I don't think the extra code for generating the file, plus loading the JSON is worth it to avoid a an extra length check. fix(postcss-colormin): do not replace original value with longer one fix #1042 feat(postcss-colormin): switch to colord * round color values fix #819 * reduce dependencies perf(postcss-colormin): improve caching Fix #771 refactor(postcss-colormin): drop the generate script

* ensure that the replacement color values are shorter than the original. Previously, the plugin returned the converted value without checking whether it was shorter than the original, we now ensure we always return the shortest color value * rework the colomin cache so use only one cache as in #771 (without using Ramda as in that PR). The cache key is now just the declaration value. Before there were two caches, one with a CSS `property|value` key and one that cached the single colors. * change the underlying library to [colord](https://github.com/omgovich/colord). Using `colord` comes with some trade-offs. On the plus side, it automatically rounds up up, which fixes an [open issue](#819) without additional work, reduces transitive dependencies from 5 to 0 (although I think the previous 4 are all maintained by the same people), is smaller and claims to be faster (haven't checked but I suspect css-value-parser is more the bottle neck than color conversion here). On the minus side, the `colord` parser is very lenient so it accepts invalid CSS colors (rgb with mixed percentages and numbers) which cssnano did not convert before, so I've added some checks to skip these values. Overall there are less lines of code. * get rid of the JSON file with precomputed CSS color names. I guess the idea was that the JSON file only contained color names that are shorter than the hex, but the color conversion libraries include all color names anyway to be able to parse, so in fact we ship the color names twice. I don't think the extra code for generating the file, plus loading the JSON is worth it to avoid a an extra length check. fix(postcss-colormin): do not replace original value with longer one fix #1042 feat(postcss-colormin): switch to colord * round color values fix #819 * reduce dependencies perf(postcss-colormin): improve caching Fix #771 refactor(postcss-colormin): drop the generate script

…1107) * ensure that the replacement color values are shorter than the original. Previously, the plugin returned the converted value without checking whether it was shorter than the original, we now ensure we always return the shortest color value * rework the colomin cache so use only one cache as in #771 (without using Ramda as in that PR). The cache key is now just the declaration value. Before there were two caches, one with a CSS `property|value` key and one that cached the single colors. * change the underlying library to [colord](https://github.com/omgovich/colord). Using `colord` comes with some trade-offs. On the plus side, it automatically rounds up up, which fixes an [open issue](#819) without additional work, reduces transitive dependencies from 5 to 0 (although I think the previous 4 are all maintained by the same people), is smaller and claims to be faster (haven't checked but I suspect css-value-parser is more the bottle neck than color conversion here). On the minus side, the `colord` parser is very lenient so it accepts invalid CSS colors (rgb with mixed percentages and numbers) which cssnano did not convert before, so I've added some checks to skip these values. Overall there are less lines of code. * get rid of the JSON file with precomputed CSS color names. I guess the idea was that the JSON file only contained color names that are shorter than the hex, but the color conversion libraries include all color names anyway to be able to parse, so in fact we ship the color names twice. I don't think the extra code for generating the file, plus loading the JSON is worth it to avoid a an extra length check. fix(postcss-colormin): do not replace original value with longer one fix #1042 feat(postcss-colormin): switch to colord * round color values fix #819 * reduce dependencies perf(postcss-colormin): improve caching Fix #771 refactor(postcss-colormin): drop the generate script

ben-eb requested a review from alexander-akait May 23, 2019 14:24

ben-eb force-pushed the refactor-colormin branch from 728d797 to 27554fb Compare May 23, 2019 20:26

ludofischer added this to In progress in colormin bugs Apr 28, 2021

ludofischer moved this from In progress to To do in colormin bugs Apr 28, 2021

ludofischer mentioned this pull request May 18, 2021

feat(postcss-colormin): switch to colord and solve multiple issues #1107

Merged

ludofischer closed this in #1107 May 19, 2021

ludofischer moved this from To do to Done in colormin bugs May 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(postcss-colormin): Improve caching. #771

refactor(postcss-colormin): Improve caching. #771

ben-eb commented May 23, 2019 •

edited

codecov-io commented May 23, 2019 •

edited

refactor(postcss-colormin): Improve caching. #771

refactor(postcss-colormin): Improve caching. #771

Conversation

ben-eb commented May 23, 2019 • edited

codecov-io commented May 23, 2019 • edited

Codecov Report

ben-eb commented May 23, 2019 •

edited

codecov-io commented May 23, 2019 •

edited