Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2797253011:
Improve json_comment_eater.py performance. (Closed)
|
Created:
3 years, 8 months ago by martijnc Modified:
3 years, 8 months ago Reviewers:
Dirk Pranke CC:
chromium-reviews, lgarron Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImprove json_comment_eater.py performance.
The JSON check in PRESUBMIT.py used to take ~3 minutes to complete on a large
JSON file (e.g.: net/http/transport_security_state_static.h). This CL brings
that down to a couple of seconds.
Profiling showed that most time was lost in the string.find() call in
_FindNextToken().
Old:
Presubmit checks took 206.2s to calculate.
New:
Presubmit checks took 3.6s to calculate.
BUG=697716
Review-Url: https://codereview.chromium.org/2797253011
Cr-Commit-Position: refs/heads/master@{#464042}
Committed: https://chromium.googlesource.com/chromium/src/+/5a78808161cff2557d53f9278d1b93bcc2495f47
Patch Set 1 #
Total comments: 2
Patch Set 2 : Improve JSON comment eater performance. #Messages
Total messages: 20 (14 generated)
The CQ bit was checked by martijn@martijnc.be to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
martijn@martijnc.be changed reviewers: + dpranke@chromium.org
Hi, can you review this patch? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm if you file a bug per the comments. https://codereview.chromium.org/2797253011/diff/1/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/2797253011/diff/1/PRESUBMIT.py#newcode1418 PRESUBMIT.py:1418: r'^net[\\\/]http[\\\/]transport_security_state_static\.json$', Seems like we should fix the comment eater instead, there must be a pathological bug in the parser for it to be this slow. However, I'm not really okay with checking things in that call themselves json but aren't actually json, so theoretically these should be .json5 or something else instead. But either of those things are more work. Can you file a bug to follow up on this and cc me on it?
Description was changed from ========== Exclude transport_security_state_static.json from JSON PRESUBMIT. The JSON check in PRESUBMIT.py takes ~3 minutes* to complete when this file is changed. This JSON file is used as input for the transport security state generator which will fail the build when invalid JSON is committed. This will give similar protection as the PRESUBMIT check. * Most time is spend removing comments from the input. BUG=697716 ========== to ========== Improve json_comment_eater.py performance. The JSON check in PRESUBMIT.py used to take ~3 minutes to complete on a large JSON file (e.g.: net/http/transport_security_state_static.h). This CL brings that down to a couple of seconds. Profiling showed that most time was lost in the string.find() call in _FindNextToken(). Old: Presubmit checks took 206.2s to calculate. New: Presubmit checks took 3.6s to calculate. BUG=697716 ==========
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by martijn@martijnc.be to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Can you take another look? I've updated the CL to fix the performance instead. Thanks! https://codereview.chromium.org/2797253011/diff/1/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/2797253011/diff/1/PRESUBMIT.py#newcode1418 PRESUBMIT.py:1418: r'^net[\\\/]http[\\\/]transport_security_state_static\.json$', On 2017/04/10 at 19:34:27, Dirk Pranke wrote: > Seems like we should fix the comment eater instead, there must be a pathological bug in the parser for it to be this slow. I've updated the CL to fix the performance issue instead. > However, I'm not really okay with checking things in that call themselves json but aren't actually json, so theoretically these should be .json5 or something else instead. > > But either of those things are more work. > > Can you file a bug to follow up on this and cc me on it? Filed https://crbug.com/710587 for the incorrect extensions but I don't have editbug privileges so I can't CC you.
Nice! Thanks for the improvements and the bug. lgtm.
The CQ bit was checked by martijn@martijnc.be
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492012334769320,
"parent_rev": "dae7b95e366ce877d08585a7b73927f96fe65d88", "commit_rev":
"5a78808161cff2557d53f9278d1b93bcc2495f47"}
Message was sent while issue was closed.
Description was changed from ========== Improve json_comment_eater.py performance. The JSON check in PRESUBMIT.py used to take ~3 minutes to complete on a large JSON file (e.g.: net/http/transport_security_state_static.h). This CL brings that down to a couple of seconds. Profiling showed that most time was lost in the string.find() call in _FindNextToken(). Old: Presubmit checks took 206.2s to calculate. New: Presubmit checks took 3.6s to calculate. BUG=697716 ========== to ========== Improve json_comment_eater.py performance. The JSON check in PRESUBMIT.py used to take ~3 minutes to complete on a large JSON file (e.g.: net/http/transport_security_state_static.h). This CL brings that down to a couple of seconds. Profiling showed that most time was lost in the string.find() call in _FindNextToken(). Old: Presubmit checks took 206.2s to calculate. New: Presubmit checks took 3.6s to calculate. BUG=697716 Review-Url: https://codereview.chromium.org/2797253011 Cr-Commit-Position: refs/heads/master@{#464042} Committed: https://chromium.googlesource.com/chromium/src/+/5a78808161cff2557d53f9278d1b... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/5a78808161cff2557d53f9278d1b... |
