New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 852050 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 27
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Performance entries should use Atomic Strings

Project Member Reported by npm@chromium.org, Jun 12 2018

Issue description

This should save lots of space: the |type| is the same string for all entries of the same type.
 
Cc: y...@yoav.ws
I think Yoav had thoughts on this?

Comment 2 by y...@yoav.ws, Jun 12 2018

Are entries being passed around between different threads? Or are worker entries created there and consumed there? I think it's the latter, but one of the disadvantages of atomic strings is that they have to stay in the thread in which they were initialized

Comment 3 by npm@chromium.org, Jun 12 2018

I think worker entries are created in the corresponding thread. One thing to note is that paint timing (and in the future Event Timing) currently uses swap promises to compute attributes, but I think that the callback is still called from the main thread?

Comment 4 by y...@yoav.ws, Jun 13 2018

> One thing to note is that paint timing (and in the future Event Timing) currently uses swap promises to compute attributes, but I think that the callback is still called from the main thread?

I think we'll need to make sure (by adding asserts?) before switching.
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e84da8c61fe8c6734d064411b491d450ccffa5d4

commit e84da8c61fe8c6734d064411b491d450ccffa5d4
Author: Nicolas Pena <npm@chromium.org>
Date: Thu Jun 14 16:44:39 2018

Check that paint timing entries are created on the main thread

This CL adds a DCHECK to make sure that PaintTiming::ReportSwapTime is
called on the main thread. This is the method that should be called
after a swap promise is concluded. This check is added in preparation of
the change to PerformanceEntry strings to AtomicString.

Bug:  852050 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Idc39c69b155c6e90fec32a3b3c0fae7fa987c378
Reviewed-on: https://chromium-review.googlesource.com/1100971
Reviewed-by: Yoav Weiss <yoav@yoav.ws>
Commit-Queue: Nicolás Peña Moreno <npm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567304}
[modify] https://crrev.com/e84da8c61fe8c6734d064411b491d450ccffa5d4/third_party/blink/renderer/core/paint/paint_timing.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Jul 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c185601b6a33871bdf6189060bcdef7c0c59321f

commit c185601b6a33871bdf6189060bcdef7c0c59321f
Author: Nicolas Pena <npm@chromium.org>
Date: Mon Jul 09 21:00:13 2018

Use AtomicString for PerformanceEntry::entryType()

This CL makes PerformanceEntry::entryType() a pure virtual method
instead of storing |entry_type_|. It also changes the usage from String
to AtomicString. Only PerformanceEventTiming keeps |entry_type_| since
it can be 'event' or 'firstInput'.

Since the entry type is no longer available from the PerformanceEntry
constructor, we change EntryTypeEnum() to pure virtual too.

Bug:  852050 
Change-Id: Ie02040b52cf17c7563667ef440bbb78ae97e64c7
Reviewed-on: https://chromium-review.googlesource.com/1113671
Commit-Queue: Nicolás Peña Moreno <npm@chromium.org>
Reviewed-by: Yoav Weiss <yoav@yoav.ws>
Reviewed-by: Timothy Dresser <tdresser@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573442}
[add] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/WebKit/LayoutTests/external/wpt/performance-timeline/get-invalid-entries.html
[add] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/WebKit/LayoutTests/external/wpt/performance-timeline/resources/worker-invalid-entries.js
[modify] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/blink/renderer/core/timing/performance.cc
[modify] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/blink/renderer/core/timing/performance.h
[modify] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/blink/renderer/core/timing/performance_entry.cc
[modify] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/blink/renderer/core/timing/performance_entry.h
[modify] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/blink/renderer/core/timing/performance_event_timing.cc
[modify] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/blink/renderer/core/timing/performance_event_timing.h
[modify] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/blink/renderer/core/timing/performance_long_task_timing.cc
[modify] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/blink/renderer/core/timing/performance_long_task_timing.h
[modify] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/blink/renderer/core/timing/performance_mark.cc
[modify] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/blink/renderer/core/timing/performance_mark.h
[modify] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/blink/renderer/core/timing/performance_measure.cc
[modify] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/blink/renderer/core/timing/performance_measure.h
[modify] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/blink/renderer/core/timing/performance_navigation_timing.cc
[modify] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/blink/renderer/core/timing/performance_navigation_timing.h
[modify] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/blink/renderer/core/timing/performance_observer.cc
[modify] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/blink/renderer/core/timing/performance_observer_entry_list.cc
[modify] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/blink/renderer/core/timing/performance_observer_entry_list.h
[modify] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/blink/renderer/core/timing/performance_paint_timing.cc
[modify] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/blink/renderer/core/timing/performance_paint_timing.h
[modify] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/blink/renderer/core/timing/performance_resource_timing.cc
[modify] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/blink/renderer/core/timing/performance_resource_timing.h
[modify] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/blink/renderer/core/timing/task_attribution_timing.cc
[modify] https://crrev.com/c185601b6a33871bdf6189060bcdef7c0c59321f/third_party/blink/renderer/core/timing/task_attribution_timing.h

Project Member

Comment 7 by bugdroid1@chromium.org, Jul 31

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/274a991d2543ddcc1bf2b58f56f8bc36cab5f753

commit 274a991d2543ddcc1bf2b58f56f8bc36cab5f753
Author: Nicolas Pena <npm@chromium.org>
Date: Tue Jul 31 22:58:49 2018

Use AtomicString for PerformanceEntry::name

This CL changes the name of a PerformanceEntry to become a name. It
should be noted that in general performance entries can be accessed from
main or worker threads, so some care is needed. An exception to this is
paint and longtask entries, and appropriate DCHECKs were added for those
cases.

Assessment of this change:
Speed: AtomicString comparisons are faster but this CL increased the
number of conversions between string types. So overall unclear, but
likely insignificant speed changes.
Memory: PerformanceEntry names are now shared so the memory usage
should be improved by this change.
Code complexity: increased (AtomicStrings are IMO harder to reason about
than slightly simpler Strings).

Bug:  852050 
Change-Id: Iaee67c6bcd6123851a0f6657d12fb6b2c61cb5fc
Reviewed-on: https://chromium-review.googlesource.com/1153608
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Timothy Dresser <tdresser@chromium.org>
Commit-Queue: Nicolás Peña Moreno <npm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579609}
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/frame/performance_monitor.cc
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/performance.cc
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/performance.h
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/performance_entry.cc
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/performance_entry.h
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/performance_event_timing.cc
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/performance_event_timing.h
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/performance_long_task_timing.cc
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/performance_long_task_timing.h
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/performance_mark.cc
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/performance_mark.h
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/performance_measure.cc
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/performance_measure.h
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/performance_navigation_timing.cc
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/performance_paint_timing.cc
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/performance_paint_timing.h
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/performance_resource_timing.cc
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/performance_resource_timing.h
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/performance_user_timing.cc
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/performance_user_timing.h
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/sub_task_attribution.cc
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/sub_task_attribution.h
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/task_attribution_timing.cc
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/task_attribution_timing.h
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/window_performance.cc
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/window_performance.h
[modify] https://crrev.com/274a991d2543ddcc1bf2b58f56f8bc36cab5f753/third_party/blink/renderer/core/timing/window_performance_test.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Aug 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/28091394686114017333c76e81b634e713588d3d

commit 28091394686114017333c76e81b634e713588d3d
Author: Nicolas Pena <npm@chromium.org>
Date: Fri Aug 24 16:25:50 2018

Add static strings for PerformanceEntry types

This CL adds static strings for the various PerformanceEntry types by using
the the make_names template. This removes the need for thread-safe copies.

Bug:  852050 
Change-Id: Ie37d3022c25bb354d335edebbc08e78de0ccbd7a
Reviewed-on: https://chromium-review.googlesource.com/1187377
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Nicolás Peña Moreno <npm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585864}
[modify] https://crrev.com/28091394686114017333c76e81b634e713588d3d/third_party/blink/renderer/core/BUILD.gn
[modify] https://crrev.com/28091394686114017333c76e81b634e713588d3d/third_party/blink/renderer/core/core_initializer.cc
[modify] https://crrev.com/28091394686114017333c76e81b634e713588d3d/third_party/blink/renderer/core/timing/performance_entry.cc
[modify] https://crrev.com/28091394686114017333c76e81b634e713588d3d/third_party/blink/renderer/core/timing/performance_entry.h
[add] https://crrev.com/28091394686114017333c76e81b634e713588d3d/third_party/blink/renderer/core/timing/performance_entry_names.json5
[modify] https://crrev.com/28091394686114017333c76e81b634e713588d3d/third_party/blink/renderer/core/timing/performance_event_timing.cc
[modify] https://crrev.com/28091394686114017333c76e81b634e713588d3d/third_party/blink/renderer/core/timing/performance_long_task_timing.cc
[modify] https://crrev.com/28091394686114017333c76e81b634e713588d3d/third_party/blink/renderer/core/timing/performance_mark.cc
[modify] https://crrev.com/28091394686114017333c76e81b634e713588d3d/third_party/blink/renderer/core/timing/performance_measure.cc
[modify] https://crrev.com/28091394686114017333c76e81b634e713588d3d/third_party/blink/renderer/core/timing/performance_navigation_timing.cc
[modify] https://crrev.com/28091394686114017333c76e81b634e713588d3d/third_party/blink/renderer/core/timing/performance_paint_timing.cc
[modify] https://crrev.com/28091394686114017333c76e81b634e713588d3d/third_party/blink/renderer/core/timing/performance_resource_timing.cc
[modify] https://crrev.com/28091394686114017333c76e81b634e713588d3d/third_party/blink/renderer/core/timing/task_attribution_timing.cc

Status: Fixed (was: Assigned)

Sign in to add a comment