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.
I am unclear why these message are labeled as "warning", which makes them sounds scary on first glance. These message are strictly informative.
<rdar://problem/36556359>
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.
Created attachment 331434 [details] Patch
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?
(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.
What is the benefit of using a try-finally block as opposed to not using one?
(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.
(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?
(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.
(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.
Created attachment 331789 [details] Alternative Patch
(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 on attachment 331789 [details] Alternative Patch Attachment 331789 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6141605 New failing tests: imported/w3c/web-platform-tests/fetch/api/redirect/redirect-mode.html
Created attachment 331797 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
The two crashes are known flakey tests and should be safe to ignore.
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 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 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__)
(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).
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.
(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.
Committed r227610: <https://trac.webkit.org/changeset/227610>
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.
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.