Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2466713002:
Make pumpEventQueue() private and explain why it is used in 'results'. (Closed)
|
Created:
4 years, 1 month ago by scheglov Modified:
4 years, 1 month ago CC:
reviews_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionMake pumpEventQueue() private and explain why it is used in 'results'.
R=brianwilkerson@google.com, paulberry@google.com
BUG=
Committed: https://github.com/dart-lang/sdk/commit/707935c6c867b9e9fe17b3f49b38880342c01c11
Patch Set 1 #
Total comments: 6
Patch Set 2 : tweak #Messages
Total messages: 13 (4 generated)
fschneider@google.com changed reviewers: + fschneider@google.com
https://codereview.chromium.org/2466713002/diff/1/pkg/analyzer/lib/src/dart/a... File pkg/analyzer/lib/src/dart/analysis/driver.dart (right): https://codereview.chromium.org/2466713002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:243: await _pumpEventQueue(128); dbc: This seems arbitrary. Why not wait on the completion of the analysis results directly?
brianwilkerson@google.com changed reviewers: - fschneider@google.com
lgtm https://codereview.chromium.org/2466713002/diff/1/pkg/analyzer/lib/src/dart/a... File pkg/analyzer/lib/src/dart/analysis/driver.dart (right): https://codereview.chromium.org/2466713002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:236: // need to be able to process update content or set priority requests "need" --> "needs" "update content" --> "updateContent" "set priority" --> "setPriorityFiles"
https://codereview.chromium.org/2466713002/diff/1/pkg/analyzer/lib/src/dart/a... File pkg/analyzer/lib/src/dart/analysis/driver.dart (right): https://codereview.chromium.org/2466713002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:236: // need to be able to process update content or set priority requests On 2016/10/31 20:41:04, Brian Wilkerson wrote: > "need" --> "needs" > "update content" --> "updateContent" > "set priority" --> "setPriorityFiles" Done. https://codereview.chromium.org/2466713002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:243: await _pumpEventQueue(128); On 2016/10/31 20:41:01, Florian Schneider wrote: > dbc: This seems arbitrary. Why not wait on the completion of the analysis > results directly? Yes, the number is arbitrary. The question does not make sense to me :-( What I would like to wait is any other asynchronous processing that requires CPU attention, i.e. does not sleep or wait for data to arrive.
On 2016/10/31 20:48:14, scheglov wrote: > https://codereview.chromium.org/2466713002/diff/1/pkg/analyzer/lib/src/dart/a... > File pkg/analyzer/lib/src/dart/analysis/driver.dart (right): > > https://codereview.chromium.org/2466713002/diff/1/pkg/analyzer/lib/src/dart/a... > pkg/analyzer/lib/src/dart/analysis/driver.dart:236: // need to be able to > process update content or set priority requests > On 2016/10/31 20:41:04, Brian Wilkerson wrote: > > "need" --> "needs" > > "update content" --> "updateContent" > > "set priority" --> "setPriorityFiles" > > Done. > > https://codereview.chromium.org/2466713002/diff/1/pkg/analyzer/lib/src/dart/a... > pkg/analyzer/lib/src/dart/analysis/driver.dart:243: await _pumpEventQueue(128); > On 2016/10/31 20:41:01, Florian Schneider wrote: > > dbc: This seems arbitrary. Why not wait on the completion of the analysis > > results directly? > > Yes, the number is arbitrary. > > The question does not make sense to me :-( > > What I would like to wait is any other asynchronous processing that requires CPU > attention, i.e. does not sleep or wait for data to arrive. lgtm for the short term. For the long term, what I really would rather see is the long-running computation moved to another isolate (actually several other isolates, so that we can take advantage of multiple CPU cores); that would address this issue because the main isolate would rarely be busy, so it would always be able to respond to requests from the client quickly. Consider adding a TODO comment to this affect.
Description was changed from ========== Make pumpEventQueue() private and explain why it is used in 'results'. R=brianwilkerson@google.com, paulberry@google.com BUG= ========== to ========== Make pumpEventQueue() private and explain why it is used in 'results'. R=brianwilkerson@google.com, paulberry@google.com BUG= Committed: https://github.com/dart-lang/sdk/commit/707935c6c867b9e9fe17b3f49b38880342c01c11 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as 707935c6c867b9e9fe17b3f49b38880342c01c11 (presubmit successful).
Message was sent while issue was closed.
fschneider@google.com changed reviewers: + fschneider@google.com
Message was sent while issue was closed.
How about removing all uses of pumpEventQueue(x) with x some magical integer constant? https://codereview.chromium.org/2466713002/diff/1/pkg/analyzer/lib/src/dart/a... File pkg/analyzer/lib/src/dart/analysis/driver.dart (right): https://codereview.chromium.org/2466713002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:243: await _pumpEventQueue(128); On 2016/10/31 20:48:14, scheglov wrote: > On 2016/10/31 20:41:01, Florian Schneider wrote: > > dbc: This seems arbitrary. Why not wait on the completion of the analysis > > results directly? > > Yes, the number is arbitrary. > > The question does not make sense to me :-( > > What I would like to wait is any other asynchronous processing that requires CPU > attention, i.e. does not sleep or wait for data to arrive. What happens if this number is too low, or too high? What I meant was that instead of pumpEventQueue, await on an the future with the result: see my CL https://codereview.chromium.org/2059503002/ which fixed issue #26556. I'm asking because I replaced several uses of pumpEventQueue in tests in the past to fix timing-dependent analyzer tests failing because some task takes a bit longer (e.g. running in debug mode). Last time I checked there were still tests that are timing-dependent.
Message was sent while issue was closed.
https://codereview.chromium.org/2466713002/diff/1/pkg/analyzer/lib/src/dart/a... File pkg/analyzer/lib/src/dart/analysis/driver.dart (right): https://codereview.chromium.org/2466713002/diff/1/pkg/analyzer/lib/src/dart/a... pkg/analyzer/lib/src/dart/analysis/driver.dart:243: await _pumpEventQueue(128); On 2016/10/31 22:16:50, Florian Schneider wrote: > On 2016/10/31 20:48:14, scheglov wrote: > > On 2016/10/31 20:41:01, Florian Schneider wrote: > > > dbc: This seems arbitrary. Why not wait on the completion of the analysis > > > results directly? > > > > Yes, the number is arbitrary. > > > > The question does not make sense to me :-( > > > > What I would like to wait is any other asynchronous processing that requires > CPU > > attention, i.e. does not sleep or wait for data to arrive. > > What happens if this number is too low, or too high? > > What I meant was that instead of pumpEventQueue, await on an the future with the > result: see my CL https://codereview.chromium.org/2059503002/ which fixed issue > #26556. > > I'm asking because I replaced several uses of pumpEventQueue in tests in the > past to fix timing-dependent analyzer tests failing because some task takes a > bit longer (e.g. running in debug mode). Last time I checked there were still > tests that are timing-dependent. Unfortunately this is a different situation from the aforementioned CL. In this CL, what's happening is that there isn't any particular result that we're waiting for. Instead, the situation is that we are trying to do a long-running computation in such a way that it appears to the user to be a background process, while allowing other code to use the same event queue to quickly handle foreground requests from the user. It would be really cool if there was a way for this code to say to the event loop "yield to all other events in the queue, and any other events they may spawn, until the event queue is completely empty. Then schedule me to continue executing". But I don't know of any way to do that. So as a poor man's substitute, we are pumping the event queue a large number of times in the hopes that any higher priority events get completely taken care of (or go into a state where they are waiting for I/O or timers) before we try to do more time consuming analysis. As I mentioned in a previous email, in the long run I would like us to fix this by moving the time-consuming analysis to its own isolate, so that the main isolate can respond quickly to user requests. But it is going to take time to rework the code to make that possible, so in the short term pumping the event queue is the best solution we've come up with.
Message was sent while issue was closed.
On 2016/10/31 22:40:33, Paul Berry wrote: > https://codereview.chromium.org/2466713002/diff/1/pkg/analyzer/lib/src/dart/a... > File pkg/analyzer/lib/src/dart/analysis/driver.dart (right): > > https://codereview.chromium.org/2466713002/diff/1/pkg/analyzer/lib/src/dart/a... > pkg/analyzer/lib/src/dart/analysis/driver.dart:243: await _pumpEventQueue(128); > On 2016/10/31 22:16:50, Florian Schneider wrote: > > On 2016/10/31 20:48:14, scheglov wrote: > > > On 2016/10/31 20:41:01, Florian Schneider wrote: > > > > dbc: This seems arbitrary. Why not wait on the completion of the analysis > > > > results directly? > > > > > > Yes, the number is arbitrary. > > > > > > The question does not make sense to me :-( > > > > > > What I would like to wait is any other asynchronous processing that requires > > CPU > > > attention, i.e. does not sleep or wait for data to arrive. > > > > What happens if this number is too low, or too high? > > > > What I meant was that instead of pumpEventQueue, await on an the future with > the > > result: see my CL https://codereview.chromium.org/2059503002/ which fixed > issue > > #26556. > > > > I'm asking because I replaced several uses of pumpEventQueue in tests in the > > past to fix timing-dependent analyzer tests failing because some task takes a > > bit longer (e.g. running in debug mode). Last time I checked there were still > > tests that are timing-dependent. > > Unfortunately this is a different situation from the aforementioned CL. In this > CL, what's happening is that there isn't any particular result that we're > waiting for. Instead, the situation is that we are trying to do a long-running > computation in such a way that it appears to the user to be a background > process, while allowing other code to use the same event queue to quickly handle > foreground requests from the user. It would be really cool if there was a way > for this code to say to the event loop "yield to all other events in the queue, > and any other events they may spawn, until the event queue is completely empty. > Then schedule me to continue executing". But I don't know of any way to do > that. So as a poor man's substitute, we are pumping the event queue a large > number of times in the hopes that any higher priority events get completely > taken care of (or go into a state where they are waiting for I/O or timers) > before we try to do more time consuming analysis. > I'm speculating, but having to work-around like this seems not optimal. Maybe there is a list of all pending requests which the code waits on for all of them to complete, before continuing the computation? > As I mentioned in a previous email, in the long run I would like us to fix this > by moving the time-consuming analysis to its own isolate, so that the main > isolate can respond quickly to user requests. But it is going to take time to > rework the code to make that possible, so in the short term pumping the event > queue is the best solution we've come up with. Agree, using isolates would be great, especially on my 32-core machine. |
