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

measure blocked layout queries #23115

Merged
merged 2 commits into from Apr 22, 2019

Conversation

Projects
None yet
4 participants
@pylbrecht
Copy link
Contributor

commented Mar 27, 2019


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #19797
  • There are tests for these changes

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Mar 27, 2019

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @jdm (or someone else) soon.

@highfive

This comment has been minimized.

Copy link

commented Mar 27, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/pipeline.rs, components/script/script_thread.rs, components/script/dom/window.rs
  • @cbrewster: components/constellation/pipeline.rs
  • @paulrouget: components/constellation/pipeline.rs
  • @KiChjang: components/script/script_thread.rs, components/script/dom/window.rs, components/script_traits/lib.rs
@highfive

This comment has been minimized.

Copy link

commented Mar 27, 2019

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@jdm
Copy link
Member

left a comment

This is the right idea, but we're currently sharing too many atomic values that should be independent. As for the panic not triggering, what pages are you testing against?

@@ -673,7 +682,12 @@ impl LayoutThread {
self.paint_time_metrics.maybe_set_metric(epoch, paint_time);
true
},
Request::FromScript(msg) => self.handle_request_helper(msg, possibly_locked_rw_data),
Request::FromScript(msg) => {
self.busy.store(true, Ordering::Relaxed);

This comment has been minimized.

Copy link
@jdm

jdm Mar 29, 2019

Member

Rather than only marking when we process script messages, we should be setting this to false before the select! and setting it to true immediately afterwards.

This comment has been minimized.

Copy link
@pylbrecht

pylbrecht Mar 31, 2019

Author Contributor

Fixed this in 8b88526.

@@ -865,6 +879,7 @@ impl LayoutThread {
self.webrender_api.clone_sender(),
self.webrender_document,
info.paint_time_metrics,
self.busy.clone(),

This comment has been minimized.

Copy link
@jdm

jdm Mar 29, 2019

Member

We should be passing an AtomicBoolean that is stored in LayoutThreadInit, rather than cloning the one that is used by the current layout thread.

This comment has been minimized.

Copy link
@jdm

jdm Apr 16, 2019

Member

My previous comment here is still valid - there's now a member of LayoutThreadInit that we should be passing here instead.


// TODO pylbrecht
// write meaningful docstring
layout_thread_is_busy: Arc<AtomicBool>,

This comment has been minimized.

Copy link
@jdm

jdm Mar 29, 2019

Member

We probably do not need to store this in ScriptThread?

This comment has been minimized.

Copy link
@pylbrecht

pylbrecht Mar 31, 2019

Author Contributor

Well, the busy flag is created in UnprivilegedPipelineContent::start_all() and passed via clone() to the layout and script thread. I'm not quite sure how to retain the relationship between the two threads, when creating Windows in ScriptThread::load(), unless storing the busy flag in ScriptThread itself.

This comment has been minimized.

Copy link
@jdm

jdm Apr 1, 2019

Member

You're right, but I think that we can store it as a member of InProgressLoad instead.

@@ -2857,6 +2864,7 @@ impl ScriptThread {
self.microtask_queue.clone(),
self.webrender_document,
self.webrender_api_sender.clone(),
self.layout_thread_is_busy.clone(),

This comment has been minimized.

Copy link
@jdm

jdm Mar 29, 2019

Member

Each Window should get its own busy boolean, since there is a 1:1 window:layout thread ratio.

This comment has been minimized.

Copy link
@pylbrecht

pylbrecht Mar 31, 2019

Author Contributor

Does this mean there is a separate layout thread for every window? To me it looks like n:1 window:layout thread, but please correct me if I'm wrong.

This comment has been minimized.

Copy link
@jdm

jdm Apr 1, 2019

Member

Each window has its own layout thread, yes.

// TODO pylbrecht
// write meaningful docstring
///
pub layout_thread_is_busy: Arc<AtomicBool>,

This comment has been minimized.

Copy link
@jdm

jdm Mar 29, 2019

Member

This probably isn't necessary given the earlier changes.

This comment has been minimized.

Copy link
@pylbrecht

pylbrecht Apr 7, 2019

Author Contributor

I feel like storing the busy flag in InitialScriptState is the only way of passing it to argument to ScriptThreadFactory::create().
If I would create it further down the line, it would not be shared across the layout and script thread. Or am I missing something here?

@pylbrecht

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

As for the panic not triggering, what pages are you testing against?

Just the one in this comment so far. Any web pages that are definitely creating blocked layout queries?

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

If you modify that page to do the layout modification and query in a loop and also add a looping CSS animation/transition, I would expect that to hit the case where the layout query starts while the layout thread is processing an animation tick from the compositor.

@pylbrecht

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

With my latest changes the only thing remaining should be creating appropriate output in Profiler::print_buckets() (and testing the final changes).

Any specifics on how I should format the output?
And should I create output for all OutputOptions?

@pylbrecht

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

I verified my changes by running ./mach run -d -- -p path/to/test.html & and checking its output.

Content of test.html:

<!DOCTYPE html>
<html>
  <head>
    <title>Test</title>
  </head>
  <body>
    <div id="test">
      test
    </div>
    <script>
      setInterval(function() {
        for (i = 0; i < 100; i++) {
          document.getElementById("test").getBoundingClientRect();
        }
      }, 200);
      setInterval(function() {
        for (i = 0; i < 100; i++) {
          document.getElementById("test").style.color = "blue";
        }
      }, 250);
    </script>
  </body>
</html>```

@pylbrecht pylbrecht force-pushed the pylbrecht:measure.blocked.layout.queries branch from 1beeef5 to 3f4f463 Apr 12, 2019

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

@pylbrecht Huh, does that page generate reports of blocked layout queries? It shouldn't as far as I can tell :<

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

Actually, I think I'm wrong - the layout thread could be busy finishing up creating the display list for the previous frame while the script thread is trying to perform a new layout query in the next frame.

@pylbrecht pylbrecht force-pushed the pylbrecht:measure.blocked.layout.queries branch from 3f4f463 to 858011c Apr 13, 2019

@pylbrecht

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

@pylbrecht Huh, does that page generate reports of blocked layout queries? It shouldn't as far as I can tell :<

Yes, it does.

Actually, I think I'm wrong - the layout thread could be busy finishing up creating the display list for the previous frame while the script thread is trying to perform a new layout query in the next frame.

I hope so, because otherwise just thinking of debugging this makes me anxious.

@@ -865,6 +879,7 @@ impl LayoutThread {
self.webrender_api.clone_sender(),
self.webrender_document,
info.paint_time_metrics,
self.busy.clone(),

This comment has been minimized.

Copy link
@jdm

jdm Apr 16, 2019

Member

My previous comment here is still valid - there's now a member of LayoutThreadInit that we should be passing here instead.


writeln!(&mut lock, "_url_\t_blocked layout queries_").unwrap();
for (url, count) in &self.blocked_layout_queries {
writeln!(&mut lock, "{}\t{}", url, count).unwrap();

This comment has been minimized.

Copy link
@jdm

jdm Apr 16, 2019

Member

Can you paste some example output that you saw?

@@ -46,6 +46,11 @@ pub enum ProfilerMsg {
),
/// Message used to force print the profiling metrics
Print,

// TODO pylbrecht
// write meaningful docstring

This comment has been minimized.

Copy link
@jdm

jdm Apr 16, 2019

Member

/// Report a layout query that could not be processed immediately for a particular URL.

@@ -202,6 +203,8 @@ struct InProgressLoad {
navigation_start_precise: u64,
/// For cancelling the fetch
canceller: FetchCanceller,
/// Flag to indicate if the layout thread is busy handling a request.

This comment has been minimized.

Copy link
@jdm

jdm Apr 16, 2019

Member

/// Flag for sharing with the layout thread that is not yet created.

@pylbrecht

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

Oh wow.. and I'm sitting here like "Why is this not merged yet?". That's really embarrassing.. (I even left a TODO in the code) ..sorry for that!

I pushed the suggested changes and will drop an example output in the comments on the weekend. Busy schedule during the week.

@pylbrecht

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

I ran the command ./mach run -d -- -z ../test.html -p for about 10 seconds and that's its output:

VMware, Inc.
llvmpipe (LLVM 7.0, 256 bits)
3.3 (Core Profile) Mesa 19.0.0
_category_                          _incremental?_ _iframe?_             _url_                  _mean (ms)_   _median (ms)_      _min (ms)_      _max (ms)_       _events_ 
Compositing                             N/A          N/A                  N/A                       59.0078          0.8786          0.0526        408.9181               7
Layout                                  yes          yes     file:///home/palbrecht/dev/tes          0.7743          0.7029          0.2757          5.7982             507
Layout                                  no           yes     file:///home/palbrecht/dev/tes         82.5015         82.5015         82.5015         82.5015               1
+ Style Recalc                          yes          yes     file:///home/palbrecht/dev/tes          3.6386          3.6386          3.6386          3.6386               1
+ Style Recalc                          no           yes     file:///home/palbrecht/dev/tes         19.0794         19.0794         19.0794         19.0794               1
| + Text Shaping                        yes          yes     file:///home/palbrecht/dev/tes          0.0000          0.0000          0.0000          0.0000               1
| + Text Shaping                        no           yes     file:///home/palbrecht/dev/tes          0.7069          0.7069          0.7069          0.7069               1
+ Restyle Damage Propagation            yes          yes     file:///home/palbrecht/dev/tes          0.0063          0.0054          0.0033          0.0292             507
+ Restyle Damage Propagation            no           yes     file:///home/palbrecht/dev/tes          0.0061          0.0061          0.0061          0.0061               1
+ Generated Content Resolution          yes          yes     file:///home/palbrecht/dev/tes          0.0102          0.0079          0.0049          0.0952             507
+ Generated Content Resolution          no           yes     file:///home/palbrecht/dev/tes          0.0104          0.0104          0.0104          0.0104               1
+ Float Placement Speculation           yes          yes     file:///home/palbrecht/dev/tes          0.0009          0.0007          0.0003          0.0248             507
+ Float Placement Speculation           no           yes     file:///home/palbrecht/dev/tes          0.0235          0.0235          0.0235          0.0235               1
+ Primary Layout Pass                   yes          yes     file:///home/palbrecht/dev/tes          0.2904          0.2904          0.2904          0.2904               1
+ Primary Layout Pass                   no           yes     file:///home/palbrecht/dev/tes          0.3777          0.3777          0.3777          0.3777               1
+ Store Overflow                        yes          yes     file:///home/palbrecht/dev/tes          0.0005          0.0004          0.0002          0.0313             507
+ Store Overflow                        no           yes     file:///home/palbrecht/dev/tes          0.0347          0.0347          0.0347          0.0347               1
+ Display List Construction             yes          yes     file:///home/palbrecht/dev/tes          0.0203          0.0059          0.0036          3.7504             507
+ Display List Construction             no           yes     file:///home/palbrecht/dev/tes          1.6878          1.6878          1.6878          1.6878               1
Script JS Evaluate                      no           yes     file:///home/palbrecht/dev/tes          1.9410          1.9410          1.9410          1.9410               1
Script Parse HTML                       no           yes     file:///home/palbrecht/dev/tes         26.8207         23.1572          0.0113         84.0995               4
Time To First Paint                     yes          yes     file:///home/palbrecht/dev/tes          0.0000          0.0000          0.0000          0.0000               1
Time To First Contentful Paint          yes          yes     file:///home/palbrecht/dev/tes          0.0000          0.0000          0.0000          0.0000               1

_url_	_blocked layout queries_
file:///home/palbrecht/dev/test.html	7

Servo exited with return value -2

The longer I wait, the more the counter of blocked layout queries increases.

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

@bors-servo r+
Looks good! Thanks for finishing this off!

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

📌 Commit 4fd9fea has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

⌛️ Testing commit 4fd9fea with merge 4e12deb...

bors-servo added a commit that referenced this pull request Apr 22, 2019

Auto merge of #23115 - pylbrecht:measure.blocked.layout.queries, r=jdm
measure blocked layout queries

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #19797

<!-- Either: -->
- [ ] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23115)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

@bors-servo bors-servo merged commit 4fd9fea into servo:master Apr 22, 2019

3 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Taskcluster (pull_request) TaskGroup: success
Details
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details

@bors-servo bors-servo referenced this pull request Apr 22, 2019

Open

Remove unnecessary explicit reflows #23118

4 of 4 tasks complete
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.