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

fix: remove prod perf markers on wc.ts #811

Merged
merged 3 commits into from
Nov 10, 2018
Merged

Conversation

jodarove
Copy link
Contributor

@jodarove jodarove commented Nov 7, 2018

Details

markers caused a bit of noise on wc-* perf tests, we can remove these cause it defeats the purpose of the performance markers in production: we dont want to measure individual components performance, which is basically what they are doing when wc is used.

summarizing… we are keeping the markers: init, hydrate for root components created with createElement, and processing rehydrate queue (all).

Does this PR introduce a breaking change?

  • Yes
  • No

@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: a5b99f1 | Target commit: 720ff20

lwc-engine-benchmark

table-append-1k metric base(a5b99f1) target(720ff20) trend
benchmark-table/append/1k duration 158.05 (±4.25 ms) 158.40 (±4.20 ms) +0.3ms (0.2%) 👌
table-clear-1k metric base(a5b99f1) target(720ff20) trend
benchmark-table/clear/1k duration 6.40 (±0.45 ms) 7.10 (±0.30 ms) +0.7ms (10.9%) 👎
table-create-10k metric base(a5b99f1) target(720ff20) trend
benchmark-table/create/10k duration 922.15 (±8.70 ms) 927.30 (±9.10 ms) +5.1ms (0.6%) 👎
table-create-1k metric base(a5b99f1) target(720ff20) trend
benchmark-table/create/1k duration 122.05 (±3.15 ms) 122.30 (±2.50 ms) +0.3ms (0.2%) 👌
table-update-10th-1k metric base(a5b99f1) target(720ff20) trend
benchmark-table/update-10th/1k duration 79.00 (±2.65 ms) 90.80 (±3.55 ms) +11.8ms (14.9%) 👎
tablecmp-append-1k metric base(a5b99f1) target(720ff20) trend
benchmark-table-component/append/1k duration 260.75 (±7.05 ms) 258.95 (±5.85 ms) -1.8ms (0.7%) 👌
tablecmp-clear-1k metric base(a5b99f1) target(720ff20) trend
benchmark-table-component/clear/1k duration 12.20 (±1.75 ms) 12.50 (±1.85 ms) +0.3ms (2.5%) 👌
tablecmp-create-10k metric base(a5b99f1) target(720ff20) trend
benchmark-table-component/create/10k duration 1811.95 (±15.75 ms) 1850.45 (±13.60 ms) +38.5ms (2.1%) 👎
tablecmp-create-1k metric base(a5b99f1) target(720ff20) trend
benchmark-table-component/create/1k duration 211.35 (±5.40 ms) 215.35 (±8.00 ms) +4.0ms (1.9%) 👎
tablecmp-update-10th-1k metric base(a5b99f1) target(720ff20) trend
benchmark-table-component/update-10th/1k duration 72.50 (±4.15 ms) 74.35 (±5.80 ms) +1.8ms (2.6%) 👌
wc-append-1k metric base(a5b99f1) target(720ff20) trend
benchmark-table-wc/append/1k duration 297.60 (±10.20 ms) 268.30 (±14.60 ms) -29.3ms (9.8%) 👍
wc-clear-1k metric base(a5b99f1) target(720ff20) trend
benchmark-table-wc/clear/1k duration 23.95 (±2.45 ms) 25.75 (±2.00 ms) +1.8ms (7.5%) 👌
wc-create-10k metric base(a5b99f1) target(720ff20) trend
benchmark-table-wc/create/10k duration 4463.70 (±27.80 ms) 2040.15 (±58.55 ms) -2423.5ms (54.3%) 👍
wc-create-1k metric base(a5b99f1) target(720ff20) trend
benchmark-table-wc/create/1k duration 267.25 (±5.30 ms) 233.60 (±4.45 ms) -33.7ms (12.6%) 👍
wc-update-10th-1k metric base(a5b99f1) target(720ff20) trend
benchmark-table-wc/update-10th/1k duration 77.00 (±6.15 ms) 76.00 (±4.20 ms) -1.0ms (1.3%) 👌

@jodarove jodarove force-pushed the jodarove/revert-marks branch from 720ff20 to 75668ed Compare November 8, 2018 07:18
@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: d520c72 | Target commit: 75668ed

lwc-engine-benchmark

table-append-1k metric base(d520c72) target(75668ed) trend
benchmark-table/append/1k duration 154.65 (±5.10 ms) 155.10 (±3.80 ms) +0.4ms (0.3%) 👌
table-clear-1k metric base(d520c72) target(75668ed) trend
benchmark-table/clear/1k duration 6.35 (±0.25 ms) 6.20 (±0.30 ms) -0.1ms (2.4%) 👍
table-create-10k metric base(d520c72) target(75668ed) trend
benchmark-table/create/10k duration 919.10 (±6.35 ms) 930.80 (±4.55 ms) +11.7ms (1.3%) 👎
table-create-1k metric base(d520c72) target(75668ed) trend
benchmark-table/create/1k duration 119.50 (±2.50 ms) 121.25 (±3.05 ms) +1.8ms (1.5%) 👎
table-update-10th-1k metric base(d520c72) target(75668ed) trend
benchmark-table/update-10th/1k duration 80.05 (±3.50 ms) 90.70 (±1.75 ms) +10.7ms (13.3%) 👎
tablecmp-append-1k metric base(d520c72) target(75668ed) trend
benchmark-table-component/append/1k duration 264.10 (±8.95 ms) 261.85 (±5.45 ms) -2.3ms (0.9%) 👌
tablecmp-clear-1k metric base(d520c72) target(75668ed) trend
benchmark-table-component/clear/1k duration 13.05 (±2.05 ms) 12.45 (±1.65 ms) -0.6ms (4.6%) 👌
tablecmp-create-10k metric base(d520c72) target(75668ed) trend
benchmark-table-component/create/10k duration 1828.30 (±9.55 ms) 1782.20 (±10.45 ms) -46.1ms (2.5%) 👍
tablecmp-create-1k metric base(d520c72) target(75668ed) trend
benchmark-table-component/create/1k duration 213.55 (±4.85 ms) 214.95 (±3.95 ms) +1.4ms (0.7%) 👌
tablecmp-update-10th-1k metric base(d520c72) target(75668ed) trend
benchmark-table-component/update-10th/1k duration 74.80 (±5.70 ms) 74.70 (±4.80 ms) -0.1ms (0.1%) 👌
wc-append-1k metric base(d520c72) target(75668ed) trend
benchmark-table-wc/append/1k duration 289.10 (±10.80 ms) 285.25 (±7.75 ms) -3.9ms (1.3%) 👌
wc-clear-1k metric base(d520c72) target(75668ed) trend
benchmark-table-wc/clear/1k duration 23.35 (±2.60 ms) 24.30 (±2.40 ms) +0.9ms (4.1%) 👌
wc-create-10k metric base(d520c72) target(75668ed) trend
benchmark-table-wc/create/10k duration 4474.00 (±21.80 ms) 4419.40 (±28.75 ms) -54.6ms (1.2%) 👍
wc-create-1k metric base(d520c72) target(75668ed) trend
benchmark-table-wc/create/1k duration 262.85 (±5.70 ms) 260.15 (±5.05 ms) -2.7ms (1.0%) 👌
wc-update-10th-1k metric base(d520c72) target(75668ed) trend
benchmark-table-wc/update-10th/1k duration 81.05 (±4.25 ms) 76.55 (±5.50 ms) -4.5ms (5.6%) 👍

avoid string concat, instead add it in the enum.
@jodarove jodarove force-pushed the jodarove/revert-marks branch from 75668ed to c7e0d3e Compare November 8, 2018 07:43
@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: d520c72 | Target commit: c7e0d3e

lwc-engine-benchmark

table-append-1k metric base(d520c72) target(c7e0d3e) trend
benchmark-table/append/1k duration 154.65 (±5.10 ms) 153.35 (±4.05 ms) -1.3ms (0.8%) 👌
table-clear-1k metric base(d520c72) target(c7e0d3e) trend
benchmark-table/clear/1k duration 6.35 (±0.25 ms) 6.55 (±0.65 ms) +0.2ms (3.1%) 👌
table-create-10k metric base(d520c72) target(c7e0d3e) trend
benchmark-table/create/10k duration 919.10 (±6.35 ms) 920.20 (±6.70 ms) +1.1ms (0.1%) 👌
table-create-1k metric base(d520c72) target(c7e0d3e) trend
benchmark-table/create/1k duration 119.50 (±2.50 ms) 118.90 (±2.25 ms) -0.6ms (0.5%) 👌
table-update-10th-1k metric base(d520c72) target(c7e0d3e) trend
benchmark-table/update-10th/1k duration 80.05 (±3.50 ms) 89.20 (±4.50 ms) +9.1ms (11.4%) 👎
tablecmp-append-1k metric base(d520c72) target(c7e0d3e) trend
benchmark-table-component/append/1k duration 264.10 (±8.95 ms) 264.25 (±6.70 ms) +0.1ms (0.1%) 👌
tablecmp-clear-1k metric base(d520c72) target(c7e0d3e) trend
benchmark-table-component/clear/1k duration 13.05 (±2.05 ms) 12.25 (±1.55 ms) -0.8ms (6.1%) 👌
tablecmp-create-10k metric base(d520c72) target(c7e0d3e) trend
benchmark-table-component/create/10k duration 1828.30 (±9.55 ms) 1814.05 (±12.65 ms) -14.2ms (0.8%) 👍
tablecmp-create-1k metric base(d520c72) target(c7e0d3e) trend
benchmark-table-component/create/1k duration 213.55 (±4.85 ms) 218.00 (±4.90 ms) +4.4ms (2.1%) 👎
tablecmp-update-10th-1k metric base(d520c72) target(c7e0d3e) trend
benchmark-table-component/update-10th/1k duration 74.80 (±5.70 ms) 72.20 (±4.80 ms) -2.6ms (3.5%) 👌
wc-append-1k metric base(d520c72) target(c7e0d3e) trend
benchmark-table-wc/append/1k duration 289.10 (±10.80 ms) 265.15 (±14.10 ms) -24.0ms (8.3%) 👍
wc-clear-1k metric base(d520c72) target(c7e0d3e) trend
benchmark-table-wc/clear/1k duration 23.35 (±2.60 ms) 23.45 (±3.25 ms) +0.1ms (0.4%) 👌
wc-create-10k metric base(d520c72) target(c7e0d3e) trend
benchmark-table-wc/create/10k duration 4474.00 (±21.80 ms) 1996.15 (±55.65 ms) -2477.8ms (55.4%) 👍
wc-create-1k metric base(d520c72) target(c7e0d3e) trend
benchmark-table-wc/create/1k duration 262.85 (±5.70 ms) 237.25 (±5.35 ms) -25.6ms (9.7%) 👍
wc-update-10th-1k metric base(d520c72) target(c7e0d3e) trend
benchmark-table-wc/update-10th/1k duration 81.05 (±4.25 ms) 78.30 (±4.60 ms) -2.8ms (3.4%) 👍

@jodarove jodarove force-pushed the jodarove/revert-marks branch from c7e0d3e to 8a1d899 Compare November 8, 2018 08:25
@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: d520c72 | Target commit: 8a1d899

lwc-engine-benchmark

table-append-1k metric base(d520c72) target(8a1d899) trend
benchmark-table/append/1k duration 154.65 (±5.10 ms) 153.25 (±4.90 ms) -1.4ms (0.9%) 👌
table-clear-1k metric base(d520c72) target(8a1d899) trend
benchmark-table/clear/1k duration 6.35 (±0.25 ms) 6.80 (±0.40 ms) +0.5ms (7.1%) 👎
table-create-10k metric base(d520c72) target(8a1d899) trend
benchmark-table/create/10k duration 919.10 (±6.35 ms) 915.10 (±6.05 ms) -4.0ms (0.4%) 👌
table-create-1k metric base(d520c72) target(8a1d899) trend
benchmark-table/create/1k duration 119.50 (±2.50 ms) 119.80 (±3.40 ms) +0.3ms (0.3%) 👌
table-update-10th-1k metric base(d520c72) target(8a1d899) trend
benchmark-table/update-10th/1k duration 80.05 (±3.50 ms) 78.60 (±3.35 ms) -1.5ms (1.8%) 👌
tablecmp-append-1k metric base(d520c72) target(8a1d899) trend
benchmark-table-component/append/1k duration 264.10 (±8.95 ms) 261.40 (±5.80 ms) -2.7ms (1.0%) 👌
tablecmp-clear-1k metric base(d520c72) target(8a1d899) trend
benchmark-table-component/clear/1k duration 13.05 (±2.05 ms) 12.10 (±2.05 ms) -1.0ms (7.3%) 👌
tablecmp-create-10k metric base(d520c72) target(8a1d899) trend
benchmark-table-component/create/10k duration 1828.30 (±9.55 ms) 1811.70 (±12.15 ms) -16.6ms (0.9%) 👍
tablecmp-create-1k metric base(d520c72) target(8a1d899) trend
benchmark-table-component/create/1k duration 213.55 (±4.85 ms) 213.05 (±5.05 ms) -0.5ms (0.2%) 👌
tablecmp-update-10th-1k metric base(d520c72) target(8a1d899) trend
benchmark-table-component/update-10th/1k duration 74.80 (±5.70 ms) 73.35 (±4.70 ms) -1.5ms (1.9%) 👌
wc-append-1k metric base(d520c72) target(8a1d899) trend
benchmark-table-wc/append/1k duration 289.10 (±10.80 ms) 293.50 (±9.75 ms) +4.4ms (1.5%) 👌
wc-clear-1k metric base(d520c72) target(8a1d899) trend
benchmark-table-wc/clear/1k duration 23.35 (±2.60 ms) 23.60 (±2.70 ms) +0.3ms (1.1%) 👌
wc-create-10k metric base(d520c72) target(8a1d899) trend
benchmark-table-wc/create/10k duration 4474.00 (±21.80 ms) 2387.35 (±32.85 ms) -2086.7ms (46.6%) 👍
wc-create-1k metric base(d520c72) target(8a1d899) trend
benchmark-table-wc/create/1k duration 262.85 (±5.70 ms) 268.75 (±6.35 ms) +5.9ms (2.2%) 👎
wc-update-10th-1k metric base(d520c72) target(8a1d899) trend
benchmark-table-wc/update-10th/1k duration 81.05 (±4.25 ms) 81.40 (±6.20 ms) +0.4ms (0.4%) 👌

@jodarove
Copy link
Contributor Author

jodarove commented Nov 8, 2018

@dbajric these are benchmarks explanation:

  1. a full revert of 24e029a.
  2. micro opt (to see how much it helped).
  3. micro opt and without wc.ts markers (wc-create-10k).
  4. and this is the interesting one, master as is (all markers), with micro opt and clearing markers and measures, this was the issue, thus most of the improvements.

i still think that we should remove the wc.ts production marks, but i wanted to check different approaches knowing that it had an issue.

thoughts?

markers caused a bit of noise on wc-* perf tests, we can remove these cause it defeats the purpose of the performance markers in production: we dont want to measure individual components performance, wich is basically what they are doing when wc are used.
summarizing… we are keeping the markers: init, hydrate for root components created with createElement, and processing rehydrate queue (all)
@jodarove jodarove changed the title Revert "feat(engine): performance marks (#756)" fix: remove prod perf markers on wc.ts Nov 8, 2018
@caridy
Copy link
Contributor

caridy commented Nov 8, 2018

what is this? why only in wc.ts? what about the other marks? wc.ts is not really used in platform today, so it is definitely not the one affecting the perf.

@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: d520c72 | Target commit: 8fba801

lwc-engine-benchmark

table-append-1k metric base(d520c72) target(8fba801) trend
benchmark-table/append/1k duration 154.65 (±5.10 ms) 155.35 (±3.75 ms) +0.7ms (0.5%) 👌
table-clear-1k metric base(d520c72) target(8fba801) trend
benchmark-table/clear/1k duration 6.35 (±0.25 ms) 6.30 (±0.30 ms) -0.0ms (0.8%) 👌
table-create-10k metric base(d520c72) target(8fba801) trend
benchmark-table/create/10k duration 919.10 (±6.35 ms) 921.95 (±6.15 ms) +2.9ms (0.3%) 👌
table-create-1k metric base(d520c72) target(8fba801) trend
benchmark-table/create/1k duration 119.50 (±2.50 ms) 117.70 (±2.90 ms) -1.8ms (1.5%) 👌
table-update-10th-1k metric base(d520c72) target(8fba801) trend
benchmark-table/update-10th/1k duration 80.05 (±3.50 ms) 79.60 (±3.00 ms) -0.5ms (0.6%) 👌
tablecmp-append-1k metric base(d520c72) target(8fba801) trend
benchmark-table-component/append/1k duration 264.10 (±8.95 ms) 259.65 (±8.15 ms) -4.5ms (1.7%) 👌
tablecmp-clear-1k metric base(d520c72) target(8fba801) trend
benchmark-table-component/clear/1k duration 13.05 (±2.05 ms) 12.35 (±1.30 ms) -0.7ms (5.4%) 👌
tablecmp-create-10k metric base(d520c72) target(8fba801) trend
benchmark-table-component/create/10k duration 1828.30 (±9.55 ms) 1799.10 (±10.60 ms) -29.2ms (1.6%) 👍
tablecmp-create-1k metric base(d520c72) target(8fba801) trend
benchmark-table-component/create/1k duration 213.55 (±4.85 ms) 212.60 (±4.95 ms) -0.9ms (0.4%) 👌
tablecmp-update-10th-1k metric base(d520c72) target(8fba801) trend
benchmark-table-component/update-10th/1k duration 74.80 (±5.70 ms) 74.00 (±4.10 ms) -0.8ms (1.1%) 👌
wc-append-1k metric base(d520c72) target(8fba801) trend
benchmark-table-wc/append/1k duration 289.10 (±10.80 ms) 253.00 (±16.75 ms) -36.1ms (12.5%) 👍
wc-clear-1k metric base(d520c72) target(8fba801) trend
benchmark-table-wc/clear/1k duration 23.35 (±2.60 ms) 24.65 (±3.35 ms) +1.3ms (5.6%) 👎
wc-create-10k metric base(d520c72) target(8fba801) trend
benchmark-table-wc/create/10k duration 4474.00 (±21.80 ms) 2031.05 (±48.95 ms) -2442.9ms (54.6%) 👍
wc-create-1k metric base(d520c72) target(8fba801) trend
benchmark-table-wc/create/1k duration 262.85 (±5.70 ms) 232.25 (±4.30 ms) -30.6ms (11.6%) 👍
wc-update-10th-1k metric base(d520c72) target(8fba801) trend
benchmark-table-wc/update-10th/1k duration 81.05 (±4.25 ms) 77.45 (±5.60 ms) -3.6ms (4.4%) 👍

@jodarove
Copy link
Contributor Author

jodarove commented Nov 8, 2018

@caridy i updated the details and desc on the pr.

you are correct, they are not affecting the perf in platform, but was a red flag. When using wc, we are measuring individual components, that we agreed we dont want.

The main issue on wc-table-10k is because the markers were not being cleared, that solved the perf regression on wc, cause again, the volume of marks and measures in this test is really high in comparison with the regular perf tests.

I think we should remove these (wc prod marks) cause we are not using them on platform, and we can always bring them back.

@jodarove jodarove removed the nomerge label Nov 8, 2018
@jodarove jodarove requested review from caridy and diervo November 8, 2018 16:46
@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: d520c72 | Target commit: 8fba801

lwc-engine-benchmark

table-append-1k metric base(d520c72) target(8fba801) trend
benchmark-table/append/1k duration 154.65 (±5.10 ms) 155.90 (±3.75 ms) +1.3ms (0.8%) 👌
table-clear-1k metric base(d520c72) target(8fba801) trend
benchmark-table/clear/1k duration 6.35 (±0.25 ms) 6.35 (±0.45 ms) 0.0ms (0.0%) 👌
table-create-10k metric base(d520c72) target(8fba801) trend
benchmark-table/create/10k duration 919.10 (±6.35 ms) 915.20 (±8.20 ms) -3.9ms (0.4%) 👍
table-create-1k metric base(d520c72) target(8fba801) trend
benchmark-table/create/1k duration 119.50 (±2.50 ms) 118.85 (±2.65 ms) -0.7ms (0.5%) 👌
table-update-10th-1k metric base(d520c72) target(8fba801) trend
benchmark-table/update-10th/1k duration 80.05 (±3.50 ms) 79.30 (±3.25 ms) -0.8ms (0.9%) 👌
tablecmp-append-1k metric base(d520c72) target(8fba801) trend
benchmark-table-component/append/1k duration 264.10 (±8.95 ms) 257.80 (±6.15 ms) -6.3ms (2.4%) 👌
tablecmp-clear-1k metric base(d520c72) target(8fba801) trend
benchmark-table-component/clear/1k duration 13.05 (±2.05 ms) 12.80 (±1.65 ms) -0.3ms (1.9%) 👌
tablecmp-create-10k metric base(d520c72) target(8fba801) trend
benchmark-table-component/create/10k duration 1828.30 (±9.55 ms) 1834.25 (±12.90 ms) +6.0ms (0.3%) 👌
tablecmp-create-1k metric base(d520c72) target(8fba801) trend
benchmark-table-component/create/1k duration 213.55 (±4.85 ms) 223.35 (±4.80 ms) +9.8ms (4.6%) 👎
tablecmp-update-10th-1k metric base(d520c72) target(8fba801) trend
benchmark-table-component/update-10th/1k duration 74.80 (±5.70 ms) 76.00 (±5.60 ms) +1.2ms (1.6%) 👌
wc-append-1k metric base(d520c72) target(8fba801) trend
benchmark-table-wc/append/1k duration 289.10 (±10.80 ms) 260.80 (±16.60 ms) -28.3ms (9.8%) 👍
wc-clear-1k metric base(d520c72) target(8fba801) trend
benchmark-table-wc/clear/1k duration 23.35 (±2.60 ms) 24.65 (±2.30 ms) +1.3ms (5.6%) 👎
wc-create-10k metric base(d520c72) target(8fba801) trend
benchmark-table-wc/create/10k duration 4474.00 (±21.80 ms) 1997.50 (±66.15 ms) -2476.5ms (55.4%) 👍
wc-create-1k metric base(d520c72) target(8fba801) trend
benchmark-table-wc/create/1k duration 262.85 (±5.70 ms) 235.50 (±4.85 ms) -27.4ms (10.4%) 👍
wc-update-10th-1k metric base(d520c72) target(8fba801) trend
benchmark-table-wc/update-10th/1k duration 81.05 (±4.25 ms) 76.45 (±4.20 ms) -4.6ms (5.7%) 👍

@diervo diervo requested a review from dbajric November 9, 2018 22:33
@diervo
Copy link
Contributor

diervo commented Nov 9, 2018

@dbajric can you take a look make sure is fine by you now?

@jodarove jodarove merged commit 074d522 into master Nov 10, 2018
@jodarove jodarove deleted the jodarove/revert-marks branch November 10, 2018 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants