Bug 168329 - webkitpy: Memoize app_identifier_from_bundle for efficiency
Summary: webkitpy: Memoize app_identifier_from_bundle for efficiency
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-02-14 13:08 PST by Jonathan Bedard
Modified: 2017-02-15 09:59 PST (History)
6 users (show)

See Also:


Attachments
Patch (2.14 KB, patch)
2017-02-14 13:22 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (1.83 KB, patch)
2017-02-15 08:53 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2017-02-14 13:08:49 PST
The DarwinPort class in webkitpy calls PlistBuddy to extract the app identifier from the bundle of an app.  This function should be memoized to prevent unneeded calls to PlistBuddy.
Comment 1 Radar WebKit Bug Importer 2017-02-14 13:10:03 PST
<rdar://problem/30518832>
Comment 2 Jonathan Bedard 2017-02-14 13:22:27 PST
Created attachment 301538 [details]
Patch
Comment 3 Daniel Bates 2017-02-14 23:22:07 PST
Comment on attachment 301538 [details]
Patch

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

> Tools/ChangeLog:7
> +

Please explain why we are making these changes. I assume we are making the memorization change because we instantiate SimulatorProcess frequently and SimulatorProcess.__init__() computes the app bundle identifier. I am assuming DarwinPort._get_crash_log() was just wrong? Regardless, please clarify. The purpose of the ChangeLog is to explain why a change was made. The only time it is acceptable to omit a description is when the title sufficiently explains the reason for a change. The title of this bug is insuffient as it only explains what we are doing and not why.

On another note, we generally prefer one bug fix per patch. Among other reasons this limits the risk of causing a regression that could result in a full rollout of the patch. When the patch fixes multiple bugs a rollout in response to a regressiion due to one of the bug fixes results in the undoing of the unrelated bug fixes (since they are in the same patch and assuming the person that performs rollout does a full revert). Using the one bug fix per patch approach at most one of the bug fixes would be reverted. This patch fixes three bugs: 1. Memorize a function 2. Fix _get_crash_log() and 3. Remove an unnecessary import of a Python module.
Comment 4 Daniel Bates 2017-02-14 23:23:02 PST
Comment on attachment 301538 [details]
Patch

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

> Tools/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=168329

Please add the Radar bug URL under this line before landing.
Comment 5 Jonathan Bedard 2017-02-15 08:38:13 PST
I will split this into the three requested bugs.  This bug will continue to track the memoization change, I will include better descriptions of the rationale of the changes in the respective change logs.
Comment 6 Jonathan Bedard 2017-02-15 08:53:06 PST
Created attachment 301622 [details]
Patch
Comment 7 Jonathan Bedard 2017-02-15 09:04:05 PST
Remove time import: https://bugs.webkit.org/show_bug.cgi?id=168371
Fix DarwinPort._get_crash_log(): https://bugs.webkit.org/show_bug.cgi?id=168372
Comment 8 WebKit Commit Bot 2017-02-15 09:59:45 PST
Comment on attachment 301622 [details]
Patch

Clearing flags on attachment: 301622

Committed r212372: <http://trac.webkit.org/changeset/212372>
Comment 9 WebKit Commit Bot 2017-02-15 09:59:54 PST
All reviewed patches have been landed.  Closing bug.