Memoize convertColor to add speed. #936
Conversation
manthey
force-pushed
the
memoize-convert-color
branch
2 times, most recently
from
b0a0b43 to
2037c67
Compare
manthey
force-pushed
the
memoize-convert-color
branch
5 times, most recently
from
ad557e0 to
2b8ae88
Compare
| * @param {geo.geoColorObject} resultColor The result of the conversion. | ||
| * @returns {geo.geoColorObject} The `resultColor`. | ||
| */ | ||
| function memoizeConvertColor(origColor, resultColor) { |
There was a problem hiding this comment.
should we call it cacheConvertColor essentially using cache instead of memoize, as it may provide consistent naming convention elsewhere, memoize is fine as well.
There was a problem hiding this comment.
I'd argue that memoization is a specific type of cache specifically when you cache just the return value of a function. Here, I think we are doing memoization. In the tile layer, we are also loading the tiles and handling asynchronous factors, so there I think it is caching but not memoization.
There was a problem hiding this comment.
Okay, lets go with memoization, I do not see the distinction but it seems that others may. +1
| */ | ||
| function memoizeConvertColor(origColor, resultColor) { | ||
| m_memoizeConvertColor.count += 1; | ||
| if (m_memoizeConvertColor.count >= m_memoizeConvertColor.maxCount) { |
There was a problem hiding this comment.
I'm not sure what you mean here. If the memoize cache has maxCount values currently, adding another value will exceed it, so we reset before adding another value.
There was a problem hiding this comment.
I meant that I liked this check
| @@ -115,6 +115,28 @@ describe('geo.util.convertColor', function () { | |||
| expect(geo.util.convertColor(undefined)).toBe(undefined); | |||
| }); | |||
| }); | |||
| describe('Memoization', function () { | |||
There was a problem hiding this comment.
for some improved reading of the test outputs, I would use Memoization Colors - as we may have more memoization in the future
There was a problem hiding this comment.
The test output looks like:
geo.util.convertColor
From strings
✓ #000000
✓ #ffffff
...
✓ TRANSPARENT
✓ none
From hex value
✓ 0x000000
✓ 0xffffff
✓ 0x1256ab
Pass through unknown colors
✓ object
✓ undefined
Memoization
✓ Response is memoized
✓ Memoization gets reset
So this is already under geo.util.convertColor. I think that it is clear enough.
manthey
force-pushed
the
memoize-convert-color
branch
from
2b8ae88 to
bb7b190
Compare
Also, refactor util/index.js to have color utilities in their own file and other common utilities in util/common.js. Named CSS colors are now taken from the npm color-name package instead of including them ourselves.
manthey
force-pushed
the
memoize-convert-color
branch
from
bb7b190 to
6e27b1a
Compare
Also, refactor util/index.js to have color utilities in their own file and other common utilities in util/common.js.