Skip to content

faster style-scope setPropertyValues#9083

Merged
NathanWalker merged 10 commits into
NativeScript:mainfrom
Akylas:faster_style_scope
Sep 7, 2022
Merged

faster style-scope setPropertyValues#9083
NathanWalker merged 10 commits into
NativeScript:mainfrom
Akylas:faster_style_scope

Conversation

@farfromrefug

Copy link
Copy Markdown
Collaborator

That Pr makes the style-scope setPropertyValues much faster. It tries not to do so many loops.
The only real change is the fact that i dont freeze the _appliedPropertyValues object anymore. I need that to go faster and as the object is not used outside of that function it does not seem to make a difference.

Now do we have tests which would ensure that is not breaking anything?

No need for so many for loops
@cla-bot cla-bot Bot added the cla: yes label Dec 2, 2020

@NathanaelA NathanaelA left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Deleted Comment, covered in Review below... -

@NathanaelA NathanaelA left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you benchmarked this against the current code, I'd be interested to know what savings (if any) you create with this refactor.

Comment thread packages/core/ui/styling/style-scope.ts Outdated

let isCssExpressionInUse = false;

const oldProperties = this._appliedPropertyValues || {};

This comment was marked as abuse.

Comment thread packages/core/ui/styling/style-scope.ts Outdated
_onDynamicStateChangeHandler: () => void;
_appliedChangeMap: Readonly<ChangeMap<ViewBase>>;
_appliedPropertyValues: Readonly<Record<string, unknown>>;
_appliedPropertyValues: Record<string, unknown>;

This comment was marked as abuse.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NathanaelA then we should make it private instead ? the readonly think makes tsc not happy when i delete properties on oldProperties

This comment was marked as abuse.


// Unset removed values
for (const property in oldProperties) {
if (!(property in newPropertyValues)) {

This comment was marked as abuse.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NathanaelA good catch. the issue is up. I mean in fact any value from newPropertyValues can be removed from oldProperties, right? So i should simply add the delete oldProperties[property]; for all values in the loop of newPropertyValues

This comment was marked as abuse.

@farfromrefug

Copy link
Copy Markdown
Collaborator Author

@NathanaelA no benchmark no. Just globally faster as you loop through new props only once and loop through less old properties.
Would be nice to have benchmarks around all that properties/CSS thing

@NathanaelA

This comment was marked as abuse.

@rigor789

rigor789 commented Dec 3, 2020

Copy link
Copy Markdown
Member

You can add some bench marks into the code; rename the existing function to _setPropertyValues and create a new setPropertyValues and add const x = Date.now(); this._setPropertyValues(...); y = Date.now(); console.log(y-x); it isn't totally precise but it will give a rough idea of the speed.

@farfromrefug @NathanaelA even simpler:

console.time('setPropertyValues')
this._setPropertyValues(...);
console.timeEnd('setPropertyValues')

Will print the time it has taken :D

@NathanWalker NathanWalker added this to the 8.4 milestone Aug 16, 2022
@NathanWalker

Copy link
Copy Markdown
Contributor

@farfromrefug I've updated this PR with latest main and looks like just 4 tests are affected by these changes:

  === ALL TESTS COMPLETE ===
  1632 OK, 4 failed
  DURATION: 22370.61 ms
  === END OF TESTS ===
  Test: STYLE.test_css_calc_and_variables FAILED: Stack - width === 10% At /value actual: '1' and expected: '0.1' differ.
  Test: STYLE.test_css_variable_fallback FAILED: undefined-css-variable-without-fallback Actual: <#0000FF>(string). Expected: <undefined>(undefined)
  Test: STYLE.test_css_variables FAILED: Cannot read property 'hex' of undefined
  Test: STYLE.test_nested_css_calc_and_variables FAILED: Stack - width === 200% At /value actual: '1' and expected: '2' differ.

Would you be able to peek sometime? We can queue this up to include in 8.4. It does appear that these changes will make the tests finish in faster time which leads me to believe there are some perf gains here.

@farfromrefug

Copy link
Copy Markdown
Collaborator Author

@NathanWalker all tests are now passing. Hope i got it all right. The issue was that css vars/exps need to be handled separately in the sense that all css vars must be evaluated before we can evaluate css expressions. I added a comment so we dont forget about it anymore.
It should not make things any slower

@NathanWalker
NathanWalker merged commit 9ccc54b into NativeScript:main Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants