Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimizations for mapped style properties #2239

Merged
merged 2 commits into from Jan 15, 2019

Conversation

Projects
None yet
2 participants
@ntfink
Copy link

commented Dec 12, 2018

This PR includes two proposed performance optimizations to styling, specifically for mapped properties.

Optimization 1:

  • Mapped properties for contexts an element doesn't have still get flagged as diffProps and are processed as such only to get "deleted" (those properties won't get set in getContextStyle) from the style later . The optimization catches this case earlier in the styling process.
  • Specific changes: In getPropertiesDiff, only consider a context if 1) the context has changed, or 2) if the element has the context and the context has mapped properties (case 2 contains the new behavior).

Optimization 2:

  • Mapped properties get recomputed and reapplied every time regardless of whether they changed or not. The optimization caches the previous, un-parsed value (could be changed to the full argHash if necessary) for a property and compares it to the return value of the mapped property on the next iteration to determine if reapplying can be skipped.
  • Specific changes: Cache the previous, un-parsed value in parse and skip properties in applyContextStyle if the newly computed return value for a mapped property matches the cached value (also temporarily cache the return value in case they don't match to avoid computing it a second time during parsing).

@maxkfranz maxkfranz self-requested a review Dec 13, 2018

@maxkfranz
Copy link
Member

left a comment

Thanks for the PR. I'm on vacation, but I wanted to respond to this within a reasonable time.

I agree with (1), but I don't see how (2) would provide a benefit over having the library consumer memoizing his function. Allowing the consumer to control caching and copying probably allows for greater control of performance. With the implementation of (2) right now, we have copying overhead that does not exist for 'some-style-prop': memoize(fn).

@@ -206,6 +206,13 @@ styfn.applyContextStyle = function( cxtMeta, cxtStyle, ele ){
// save cycles when the context prop doesn't need to be applied
if( eleProp === cxtProp ){ continue; }

// save cycles when a mapped context prop doesn't need to be applied
if( is.fn( cxtProp.value )) {

This comment has been minimized.

Copy link
@maxkfranz

maxkfranz Dec 21, 2018

Member

Generally if cxtProp.mapped != null

The function case is probably cheaper as cxtProp.mapped === types.fn

This comment has been minimized.

Copy link
@ntfink

ntfink Jan 11, 2019

Author

Makes sense!

@@ -35,6 +35,11 @@ styfn.parse = function( name, value, propIsBypass, propIsFlat ){
}
}

// cache the original, unparsed value for comparison on the next iteration
if( ret ){
ret.previousValue = util.copy( value );

This comment has been minimized.

Copy link
@maxkfranz

maxkfranz Dec 21, 2018

Member

Is the copying strictly necessary? It adds expense.

It's certainly not necessary in the non-mapper cases.

This comment has been minimized.

Copy link
@ntfink

ntfink Jan 11, 2019

Author

I was making an assumption that the un-parsed value could be a type (ex: array) that would require copying. But if that's not the case, I can certainly remove it!

Alternatively and/or in addition to, another option would be to move this statement to applyParsedProperty:

case types.fn:
  ...
  flatProp = this.parse( prop.name, fnRetVal, prop.bypass, flatPropMapping );
  flatProp.previousValue = fnRetVal; // or util.copy(fnRetVal)

Then regardless of whether copying is required or not, only previous values for mapper cases get cached. Which would make sense as that's the only case where it gets used.

This comment has been minimized.

Copy link
@maxkfranz

maxkfranz Jan 11, 2019

Member

If the goal is to avoid the expense of applyParsedProperty() for function mappers, could we use an early exit condition within applyParsedProperty()? Then all the logic for this purpose would be contained in the same function.

Additionally, the top-level parse() function is just about caching the properties to avoid parsing expense. It's the parseImpl() function that contains all of the real parsing logic.

It's also worth considering that line 39 would copy for all property types, whereas the code in your comment is better scoped: Copying happens only for function property values in your comment.

@@ -74,7 +74,7 @@ styfn.getPropertiesDiff = function( oldCxtKey, newCxtKey ){
let cxtHasDiffed = oldHasCxt !== newHasCxt;
let cxtHasMappedProps = cxt.mappedProperties.length > 0;

if( cxtHasDiffed || cxtHasMappedProps ){
if( cxtHasDiffed || ( newHasCxt && cxtHasMappedProps )){

This comment has been minimized.

Copy link
@maxkfranz

maxkfranz Dec 21, 2018

Member

Makes sense


if( cxtProp.fnValue === eleProp.previousValue ){ continue; }
}

This comment has been minimized.

Copy link
@maxkfranz

maxkfranz Dec 21, 2018

Member

If we use this approach, we must delete ele.updateMappers() and style.updateMappers().

This comment has been minimized.

Copy link
@ntfink

ntfink Jan 11, 2019

Author

I'm afraid I'm not too familiar with these functions, what makes them incompatible with this approach?

This comment has been minimized.

Copy link
@maxkfranz

maxkfranz Jan 11, 2019

Member

I won't go into too much detail, because they should be removed in any case: They are internal functions to update only mappers, bypassing the rest of the style checks. They were used at one point, but they don't seem to be used anymore. They should be removed as a part of this PR to avoid incompatibilities.

@maxkfranz maxkfranz self-assigned this Jan 11, 2019

@ntfink

This comment has been minimized.

Copy link
Author

commented Jan 11, 2019

And then I was on vacation and failed to respond in a reasonable time - sorry for the delay!

Thanks much for the in depth feedback! I added a couple in-line comments, but also wanted to address your more general question about (2) vs. memoize. I think they both contribute to improved performance but solve different problems.

Memoize would definitely speed up the actual computation of the property value via its caching functionality. The performance gains from (2) are not from caching the previous value (which is just a means to compare it to the new value), but from from bypassing the applyParsedProperty function chain for mapped properties that haven't changed since the last style update which you can't achieve with memoize at the consumer level. For large graphs with complicated (i.e., lots of mapped properties), cutting out that one function chain for each element adds up.

That being said, I do recognize the added overhead of copying (if it ends up being necessary). But (2) would still provide performance gains, even for memoized style properties, since they're impacting different parts of the process.

Does that make any more sense than before?

@maxkfranz

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

That helps to clarify things. As described in the code comment, if possible, I'd like to see all the logic within applyParsedProperty() with an early exit condition. That would keep all the logic in one place, but maybe it's not practical. I'll take a closer look on Monday.

@maxkfranz maxkfranz added this to the 3.4.0 milestone Jan 15, 2019

@maxkfranz maxkfranz merged commit 759971a into cytoscape:unstable Jan 15, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

maxkfranz added a commit that referenced this pull request Jan 15, 2019

Cheaper function check
Ref : Optimizations for mapped style properties #2239

maxkfranz added a commit that referenced this pull request Jan 15, 2019

- Save the previous value in `applyParsedProperty()`, only for functi…
…on mappers.

- Comparing to `eleProp.previousValue` doesn't make sense, as it's always the same as `eleProp.value`, and generally new `eleProps` can be created for each flat value.  They aren't generally re-used.
- Also make sure that the `cxtProp` and `eleProp` correspond to each other --- the `eleProp` should be a flat prop of the function mapper `cxtProp`.
- Avoid copying prop values.'

Ref : Optimizations for mapped style properties #2239

maxkfranz added a commit that referenced this pull request Jan 15, 2019

Remove internal functions: `ele.updateMappers()` and `style.updateMap…
…pers()`

Ref : Optimizations for mapped style properties #2239

maxkfranz added a commit that referenced this pull request Jan 15, 2019

Merge branch 'ntfink-mapped_style_optimizations' into unstable
Ref : Optimizations for mapped style properties #2239
@maxkfranz

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

@ntfink The changes only include the proposals that we discussed. The overall structure is the same as your initial PR.

Thanks again for the PR, and you should see the change in the 3.4.0 release at the beginning of February.

maxkfranz added a commit that referenced this pull request Feb 11, 2019

When applying a function mapper to an element, the passed parsed prop…
…erty should be copied.

- The feature to improve performance of function mappers (#2239) does not take into account that the `cxtProp` is the same object for all elements:  The `cxtProp` is a reference in the stylesheet.  Because `cxtProp` is the same for all elements, storing `fnValue` and `prevFnValue` under `cxtProp` does not make sense.  This can result in the same value being applied to all elements to which the style block applies.
- To remedy this, a copy of the `cxtProp` should be referenced by `eleProp.mapping` rather than the original `cxtProp`.  Then `fnValue` and `prevFnValue` can safely be stored per-element under `eleProp.mapping`.
- Aside: `fnRetVal` should be explicitly checked against `null`, as a nully value (e.g. empty string or zero) could cause unnecessary `fn()` calls.
- Applies to v3.4.x.

Ref #2282 #2239

maxkfranz added a commit that referenced this pull request Feb 11, 2019

When applying a function mapper to an element, the passed parsed prop…
…erty should be copied.

- The feature to improve performance of function mappers (#2239) does not take into account that the `cxtProp` is the same object for all elements:  The `cxtProp` is a reference in the stylesheet.  Because `cxtProp` is the same for all elements, storing `fnValue` and `prevFnValue` under `cxtProp` does not make sense.  This can result in the same value being applied to all elements to which the style block applies.
- To remedy this, a copy of the `cxtProp` should be referenced by `eleProp.mapping` rather than the original `cxtProp`.  Then `fnValue` and `prevFnValue` can safely be stored per-element under `eleProp.mapping`.
- Aside: `fnRetVal` should be explicitly checked against `null`, as a nully value (e.g. empty string or zero) could cause unnecessary `fn()` calls.
- Applies to v3.4.x.

Ref #2282 #2239
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.