Bug 181702 - webkit-patch upload emits irrelevant simulator warnings
Summary: webkit-patch upload emits irrelevant simulator warnings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Local Build
Hardware: Mac macOS 10.13
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-16 13:36 PST by Daniel Bates
Modified: 2018-01-29 11:35 PST (History)
9 users (show)

See Also:


Attachments
Patch (1.91 KB, patch)
2018-01-16 14:52 PST, Jonathan Bedard
jbedard: commit-queue-
Details | Formatted Diff | Diff
Alternative Patch (6.96 KB, patch)
2018-01-19 14:39 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.39 MB, application/zip)
2018-01-19 15:38 PST, EWS Watchlist
no flags Details
Alternative Alternative Patch: Remove the warning (3.64 KB, patch)
2018-01-24 17:06 PST, Daniel Bates
aakash_jain: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2018-01-16 13:36:30 PST
Today when I ran `Tools/Scripts/webkit-patch upload -g HEAD -m 'Patch and layout test' 181693` webkit-patch emitted the following warning messages:

    WARNING: This machine could support 6 simulators, but is only configured for 5.
    WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>.

These message are irrelevant to process of uploading a patch and we should not emit such warnings.
Comment 1 Daniel Bates 2018-01-16 13:41:38 PST
I am unclear why these message are labeled as "warning", which makes them sounds scary on first glance. These message are strictly informative.
Comment 2 Radar WebKit Bug Importer 2018-01-16 13:46:06 PST
<rdar://problem/36556359>
Comment 3 Jonathan Bedard 2018-01-16 14:22:34 PST
This message has always been classified as a warning. I suppose the argument could be made that such a message should be 'INFO' instead.

In any case, we shouldn't be emitting these when style checking.
Comment 4 Jonathan Bedard 2018-01-16 14:52:11 PST
Created attachment 331434 [details]
Patch
Comment 5 Daniel Bates 2018-01-17 11:33:22 PST
Comment on attachment 331434 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331434&action=review

> Tools/Scripts/webkitpy/style/checkers/test_expectations.py:133
> +        try:
> +            previous_level = logging.getLogger().level
> +            logging.getLogger().setLevel(logging.ERROR)
> +            ports_to_lint = [host.port_factory.get(name) for name in host.port_factory.all_port_names()]
> +        finally:
> +            logging.getLogger().setLevel(previous_level)

This will silence a thrown exception. How did you come to the decision to do this?
Comment 6 Jonathan Bedard 2018-01-17 11:39:36 PST
(In reply to Daniel Bates from comment #5)
> Comment on attachment 331434 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331434&action=review
> 
> > Tools/Scripts/webkitpy/style/checkers/test_expectations.py:133
> > +        try:
> > +            previous_level = logging.getLogger().level
> > +            logging.getLogger().setLevel(logging.ERROR)
> > +            ports_to_lint = [host.port_factory.get(name) for name in host.port_factory.all_port_names()]
> > +        finally:
> > +            logging.getLogger().setLevel(previous_level)
> 
> This will silence a thrown exception. How did you come to the decision to do
> this?

'A finally clause is always executed before leaving the try statement, whether an exception has occurred or not. When an exception has occurred in the try clause and has not been handled by an except clause (or it has occurred in an except or else clause), it is re-raised after the finally clause has been executed. The finally clause is also executed “on the way out” when any other clause of the try statement is left via a break, continue or return statement.'

https://docs.python.org/3/tutorial/errors.html

Finally clauses do not silence exceptions.  This is a perfect example of when we want to use a 'finally' clause. Even in the case were an exception is thrown, the logger will be returned to it's state before the exception is raised.
Comment 7 Daniel Bates 2018-01-17 12:21:30 PST
What is the benefit of using a try-finally block as opposed to not using one?
Comment 8 Jonathan Bedard 2018-01-17 12:57:33 PST
(In reply to Daniel Bates from comment #7)
> What is the benefit of using a try-finally block as opposed to not using one?

If we don't use a try-finally and an exception is raised, the logger level will not be set to it's previous value.  This means that a caller of this function may catch the exception but now find itself with an unexpected logging level.

As an example, if a user were to run 'check-webkit-style -v' and an exception were thrown here without the use of a try-finally block, a caller which catches the exception will now find itself with a logging level of logging.ERROR instead of logging.DEBUG as expected.
Comment 9 Daniel Bates 2018-01-19 11:00:24 PST
(In reply to Jonathan Bedard from comment #8)
> (In reply to Daniel Bates from comment #7)
> > What is the benefit of using a try-finally block as opposed to not using one?
> 
> If we don't use a try-finally and an exception is raised, the logger level
> will not be set to it's previous value.  This means that a caller of this
> function may catch the exception but now find itself with an unexpected
> logging level.
> 
> As an example, if a user were to run 'check-webkit-style -v' and an
> exception were thrown here without the use of a try-finally block, a caller
> which catches the exception will now find itself with a logging level of
> logging.ERROR instead of logging.DEBUG as expected.

I do not see much value in making this behave like this because it adds code and only helps people that have bad programming habits. I mean is there any recoverable exception that this code will allow to propagate? I cannot think of any at the moment.

Instead of taking the approach of silencing logging could we move the warning messages to some code path more directly relating to running tests such that check-webkit-style() does trigger it?
Comment 10 Jonathan Bedard 2018-01-19 13:13:25 PST
(In reply to Daniel Bates from comment #9)
> (In reply to Jonathan Bedard from comment #8)
> > ...
> 
> I do not see much value in making this behave like this because it adds code
> and only helps people that have bad programming habits. I mean is there any
> recoverable exception that this code will allow to propagate? I cannot think
> of any at the moment.
> 
> Instead of taking the approach of silencing logging could we move the
> warning messages to some code path more directly relating to running tests
> such that check-webkit-style() does trigger it?

It preserves the expected logging behavior in the case of an exception, which I think is very important given that this code change is very likely increasing, rather than decreasing, the logging threshold, meaning we could potentially lose logs.  Currently, we don't throw recoverable exceptions in this code path, but it seems bad to add code that would forbid this, especially since neither the port nor the style checker make such demands at present.

To your second point, no, we cannot move the warning messages, at least not without a large refactor of the port class. What you are getting at is an architectural flaw in the port class. We have the port class handle test expectations, building, Layout Test specific set-up and teardown and platform specific tooling (things like crash logs). The port class should really be separated into 4 pieces, but that is a refactor I won't have a chance to explore for a few months.
Comment 11 Daniel Bates 2018-01-19 14:38:53 PST
(In reply to Jonathan Bedard from comment #10)
> (In reply to Daniel Bates from comment #9)
> > (In reply to Jonathan Bedard from comment #8)
> > > ...
> > 
> > I do not see much value in making this behave like this because it adds code
> > and only helps people that have bad programming habits. I mean is there any
> > recoverable exception that this code will allow to propagate? I cannot think
> > of any at the moment.
> > 
> > Instead of taking the approach of silencing logging could we move the
> > warning messages to some code path more directly relating to running tests
> > such that check-webkit-style() does trigger it?
> 
> It preserves the expected logging behavior in the case of an exception,
> which I think is very important given that this code change is very likely
> increasing, rather than decreasing, the logging threshold, meaning we could
> potentially lose logs.  Currently, we don't throw recoverable exceptions in
> this code path, but it seems bad to add code that would forbid this,
> especially since neither the port nor the style checker make such demands at
> present.
> 
> To your second point, no, we cannot move the warning messages, at least not
> without a large refactor of the port class. What you are getting at is an
> architectural flaw in the port class. We have the port class handle test
> expectations, building, Layout Test specific set-up and teardown and
> platform specific tooling (things like crash logs). The port class should
> really be separated into 4 pieces, but that is a refactor I won't have a
> chance to explore for a few months.

From debugging the only reason we emit these warnings is because we call SimulatedDeviceManager.max_supported_simulators() from the IOSSimulatorPort constructor as part of logic to check the parsed value for the command line option --child-processes. The port constructor does not seem like the appropriate place for such logic. The functions _set_up_derived_options() defined in Scripts/webkitpy/layout_tests/run_webkit_tests.py seems like a more appropriate place because it is responsible for picking a default value for --child-processes if this option is not specified.
Comment 12 Daniel Bates 2018-01-19 14:39:45 PST
Created attachment 331789 [details]
Alternative Patch
Comment 13 Jonathan Bedard 2018-01-19 15:10:37 PST
(In reply to Daniel Bates from comment #11)
> (In reply to Jonathan Bedard from comment #10)
> > ...
> 
> From debugging the only reason we emit these warnings is because we call
> SimulatedDeviceManager.max_supported_simulators() from the IOSSimulatorPort
> constructor as part of logic to check the parsed value for the command line
> option --child-processes. The port constructor does not seem like the
> appropriate place for such logic. The functions _set_up_derived_options()
> defined in Scripts/webkitpy/layout_tests/run_webkit_tests.py seems like a
> more appropriate place because it is responsible for picking a default value
> for --child-processes if this option is not specified.

The downside with _set_up_derived_options owning that logging statement is it is a logging statement which is really only relevant for iOS Simulator. I think it makes it harder to trace the reason this logic was added in the first place.

I think the proposed change in attachment 331789 [details] will make the port class easier to use outside of using layout tests, however.  Which seems like an acceptable trade-off.
Comment 14 EWS Watchlist 2018-01-19 15:38:33 PST Comment hidden (obsolete)
Comment 15 EWS Watchlist 2018-01-19 15:38:34 PST Comment hidden (obsolete)
Comment 16 Jonathan Bedard 2018-01-24 10:48:48 PST
The two crashes are known flakey tests and should be safe to ignore.
Comment 17 Daniel Bates 2018-01-24 17:06:07 PST
Created attachment 332213 [details]
Alternative Alternative Patch: Remove the warning

Thinking about this issue some more, I do not feel that this warning adds much value and we should remove it.
Comment 18 Aakash Jain 2018-01-24 17:17:41 PST
Comment on attachment 332213 [details]
Alternative Alternative Patch: Remove the warning

Removing this warning seems fine.

However, the bug originally refers to another warning: "WARNING: This machine could support 6 simulators, but is only configured for 5."
Comment 19 Jonathan Bedard 2018-01-24 17:21:36 PST
Comment on attachment 332213 [details]
Alternative Alternative Patch: Remove the warning

View in context: https://bugs.webkit.org/attachment.cgi?id=332213&action=review

I'm good with this change.  I added this logging in the first place, Dan has convinced me that we don't need it.

> Tools/ChangeLog:23
> +        (IOSSimulatorPort.__init__.is):

Unclear why this is (IOSSimulatorPort.__init__.is) instead of just (IOSSimulatorPort.__init__)
Comment 20 Jonathan Bedard 2018-01-24 17:22:59 PST
(In reply to Aakash Jain from comment #18)
> Comment on attachment 332213 [details]
> Alternative Alternative Patch: Remove the warning
> 
> Removing this warning seems fine.
> 
> However, the bug originally refers to another warning: "WARNING: This
> machine could support 6 simulators, but is only configured for 5."

That warning is generated from a function called in the code Dan is deleting (SimulatedDeviceManager.max_supported_simulators(self.host), specifically).
Comment 21 Dean Johnson 2018-01-24 17:23:47 PST
I agree with Dan's approach here, but also think we should print a warning message ONLY in the case where a user specifies --child-processes >= max supported simulators for a given process limit. This is because if we spawn too many simulators with a low system process limit we can easily get a machine into a state where tests fail unexpectedly and that a reboot may be required to get your machine back into a usable state.
Comment 22 Daniel Bates 2018-01-25 09:39:30 PST
(In reply to Dean Johnson from comment #21)
> [...] we should print a warning message ONLY in the case where a user specifies --child-processes >= max supported simulators for a given process limit. This is because if we spawn too many simulators with a low system process limit we can easily get a machine into a state where tests fail unexpectedly and that a reboot may be required to get your machine back into a usable state.

Notice that we currently print this message when --child-processes > max supported simulators and we only print it once before we start running tests. This message is non-sensical because it can only be printed when a human explicitly specifies --child-processes. It seems reasonable to interpret that a person that specifies this command line option knows what they are doing and hence the message serves only to point out what they already know. (Elaborating further, it is expected that a person that specifies --child-processes has estimated their system load and knows whether or not their system can handle the specified number of child processes). We know how a person can avoid "a low system process limit" from the get-go; do not specify --child-processes and run-webkit-tests will determine the number of parallel instances based on the system load.

For completeness, "Alternative Patch" (attachment #331789 [details]) fixes this bug and without removing the message.
Comment 23 Daniel Bates 2018-01-25 09:48:57 PST
Committed r227610: <https://trac.webkit.org/changeset/227610>
Comment 24 Alexey Proskuryakov 2018-01-26 10:04:38 PST
Comment on attachment 332213 [details]
Alternative Alternative Patch: Remove the warning

View in context: https://bugs.webkit.org/attachment.cgi?id=332213&action=review

> Tools/ChangeLog:17
> +        by specifying --child-processes knows what they are doing. The effects of picking a large
> +        value be obvious, the system may become sluggish.

I don't think that this is quite accurate. The effects of running over process limits or open file limits will be very unpredictable. It will be definitely causing very weird failures in simulators, and it can potentially even cause loss of data in unrelated applications running on the system.
Comment 25 Jonathan Bedard 2018-01-29 11:35:33 PST
Both Dean and Alexey seem to be of the opinion that we should warn the user given that running too many simulators can cause issues which are not intuitive.  I've filed <https://bugs.webkit.org/show_bug.cgi?id=182255> about this, and we can continue that discussion there.