Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2558153002:
Change ExecLinkWrapper to not buffer all tool output (Closed)
|
Created:
4 years ago by brucedawson Modified:
4 years ago Reviewers:
scottmg CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange ExecLinkWrapper to not buffer all tool output
/verbose linking of chrome.dll creates over one GB of output. This
causes ExecLinkWrapper to consume over two GB of memory which leads to
an OOM failure in the 32-bit depot_tools python, and the loss of all
of the valuable output. This change modifies ExecLinkWrapper to
process the output one line at a time, thus avoiding the OOM.
I've tested that this handles the 1.1 GB of output which the previous
version of this function failed on and I've visually confirmed that the
output looks the same - no extraneous blank lines, for instance, when
displaying warnings, errors, or 1.9 million lines of verbose output.
I also verified that the script stays idle when waiting for output -
blocking on .readline().
BUG=672182
Committed: https://crrev.com/5d0d1b0bf01acb3ebe3a5ef27ebf2f3180ab1d1d
Cr-Commit-Position: refs/heads/master@{#437126}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Simpler and more idiomatic #Messages
Total messages: 25 (17 generated)
The CQ bit was checked by brucedawson@chromium.org 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.
Description was changed from ========== Change ExecLinkWrapper to not buffer all tool output When /verbose linking creates over one GB of output this causes ExecLinkWrapper to consume over two GB of memory which leads to an OOM failure in the 32-bit depot_tools python, and the loss of all of the valuable output. This changes ExecLinkWrapper to process the output one line at a time. BUG=672182 ========== to ========== Change ExecLinkWrapper to not buffer all tool output When /verbose linking creates over one GB of output this causes ExecLinkWrapper to consume over two GB of memory which leads to an OOM failure in the 32-bit depot_tools python, and the loss of all of the valuable output. This changes ExecLinkWrapper to process the output one line at a time. I've tested that this handles the 1.1 GB of output which the previous version of this function failed on and I've visuall confirmed that the output looks the same - no extraneous blank lines, for instance, when displaying warnings, errors, or millions of lines of verbose output. BUG=672182 ==========
Description was changed from ========== Change ExecLinkWrapper to not buffer all tool output When /verbose linking creates over one GB of output this causes ExecLinkWrapper to consume over two GB of memory which leads to an OOM failure in the 32-bit depot_tools python, and the loss of all of the valuable output. This changes ExecLinkWrapper to process the output one line at a time. I've tested that this handles the 1.1 GB of output which the previous version of this function failed on and I've visuall confirmed that the output looks the same - no extraneous blank lines, for instance, when displaying warnings, errors, or millions of lines of verbose output. BUG=672182 ========== to ========== Change ExecLinkWrapper to not buffer all tool output When /verbose linking creates over one GB of output this causes ExecLinkWrapper to consume over two GB of memory which leads to an OOM failure in the 32-bit depot_tools python, and the loss of all of the valuable output. This changes ExecLinkWrapper to process the output one line at a time. I've tested that this handles the 1.1 GB of output which the previous version of this function failed on and I've visuall confirmed that the output looks the same - no extraneous blank lines, for instance, when displaying warnings, errors, or 1.9 million lines of verbose output. BUG=672182 ==========
Description was changed from ========== Change ExecLinkWrapper to not buffer all tool output When /verbose linking creates over one GB of output this causes ExecLinkWrapper to consume over two GB of memory which leads to an OOM failure in the 32-bit depot_tools python, and the loss of all of the valuable output. This changes ExecLinkWrapper to process the output one line at a time. I've tested that this handles the 1.1 GB of output which the previous version of this function failed on and I've visuall confirmed that the output looks the same - no extraneous blank lines, for instance, when displaying warnings, errors, or 1.9 million lines of verbose output. BUG=672182 ========== to ========== Change ExecLinkWrapper to not buffer all tool output When /verbose linking creates over one GB of output this causes ExecLinkWrapper to consume over two GB of memory which leads to an OOM failure in the 32-bit depot_tools python, and the loss of all of the valuable output. This changes ExecLinkWrapper to process the output one line at a time. I've tested that this handles the 1.1 GB of output which the previous version of this function failed on and I've visuall confirmed that the output looks the same - no extraneous blank lines, for instance, when displaying warnings, errors, or 1.9 million lines of verbose output. I also verified that the script stays idle when waiting for output - blocking on .readline(). BUG=672182 ==========
brucedawson@chromium.org changed reviewers: + scottmg@chromium.org
WDYT? This makes /verbose work reliably again.
https://codereview.chromium.org/2558153002/diff/1/build/toolchain/win/tool_wr... File build/toolchain/win/tool_wrapper.py (right): https://codereview.chromium.org/2558153002/diff/1/build/toolchain/win/tool_wr... build/toolchain/win/tool_wrapper.py:140: if len(line) == 0: # Read of zero bytes means EOF I'm not sure about the loop termination... for line in link.stdout: if (not ...): print line, link.wait() should be the same though and a bit nicer looking?
The CQ bit was checked by brucedawson@chromium.org 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...
Much simpler now. Try bots are running but it is passing the quick local tests at least. https://codereview.chromium.org/2558153002/diff/1/build/toolchain/win/tool_wr... File build/toolchain/win/tool_wrapper.py (right): https://codereview.chromium.org/2558153002/diff/1/build/toolchain/win/tool_wr... build/toolchain/win/tool_wrapper.py:140: if len(line) == 0: # Read of zero bytes means EOF Agreed. I started with a solution that read stderr and stdout and didn't remove the complexity when I changed it to stdout only.
lgtm if it works. :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Change ExecLinkWrapper to not buffer all tool output When /verbose linking creates over one GB of output this causes ExecLinkWrapper to consume over two GB of memory which leads to an OOM failure in the 32-bit depot_tools python, and the loss of all of the valuable output. This changes ExecLinkWrapper to process the output one line at a time. I've tested that this handles the 1.1 GB of output which the previous version of this function failed on and I've visuall confirmed that the output looks the same - no extraneous blank lines, for instance, when displaying warnings, errors, or 1.9 million lines of verbose output. I also verified that the script stays idle when waiting for output - blocking on .readline(). BUG=672182 ========== to ========== Change ExecLinkWrapper to not buffer all tool output /verbose linking of chrome.dll creates over one GB of output. This causes ExecLinkWrapper to consume over two GB of memory which leads to an OOM failure in the 32-bit depot_tools python, and the loss of all of the valuable output. This change modifies ExecLinkWrapper to process the output one line at a time, thus avoiding the OOM. I've tested that this handles the 1.1 GB of output which the previous version of this function failed on and I've visually confirmed that the output looks the same - no extraneous blank lines, for instance, when displaying warnings, errors, or 1.9 million lines of verbose output. I also verified that the script stays idle when waiting for output - blocking on .readline(). BUG=672182 ==========
The CQ bit was checked by brucedawson@chromium.org
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": 20001, "attempt_start_ts": 1481156954217650,
"parent_rev": "206434b1fc423fc9957fe9e40759c99a9e308732", "commit_rev":
"1a9437c479bb903d2f1366b8aae272d1e1351c12"}
Message was sent while issue was closed.
Description was changed from ========== Change ExecLinkWrapper to not buffer all tool output /verbose linking of chrome.dll creates over one GB of output. This causes ExecLinkWrapper to consume over two GB of memory which leads to an OOM failure in the 32-bit depot_tools python, and the loss of all of the valuable output. This change modifies ExecLinkWrapper to process the output one line at a time, thus avoiding the OOM. I've tested that this handles the 1.1 GB of output which the previous version of this function failed on and I've visually confirmed that the output looks the same - no extraneous blank lines, for instance, when displaying warnings, errors, or 1.9 million lines of verbose output. I also verified that the script stays idle when waiting for output - blocking on .readline(). BUG=672182 ========== to ========== Change ExecLinkWrapper to not buffer all tool output /verbose linking of chrome.dll creates over one GB of output. This causes ExecLinkWrapper to consume over two GB of memory which leads to an OOM failure in the 32-bit depot_tools python, and the loss of all of the valuable output. This change modifies ExecLinkWrapper to process the output one line at a time, thus avoiding the OOM. I've tested that this handles the 1.1 GB of output which the previous version of this function failed on and I've visually confirmed that the output looks the same - no extraneous blank lines, for instance, when displaying warnings, errors, or 1.9 million lines of verbose output. I also verified that the script stays idle when waiting for output - blocking on .readline(). BUG=672182 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Change ExecLinkWrapper to not buffer all tool output /verbose linking of chrome.dll creates over one GB of output. This causes ExecLinkWrapper to consume over two GB of memory which leads to an OOM failure in the 32-bit depot_tools python, and the loss of all of the valuable output. This change modifies ExecLinkWrapper to process the output one line at a time, thus avoiding the OOM. I've tested that this handles the 1.1 GB of output which the previous version of this function failed on and I've visually confirmed that the output looks the same - no extraneous blank lines, for instance, when displaying warnings, errors, or 1.9 million lines of verbose output. I also verified that the script stays idle when waiting for output - blocking on .readline(). BUG=672182 ========== to ========== Change ExecLinkWrapper to not buffer all tool output /verbose linking of chrome.dll creates over one GB of output. This causes ExecLinkWrapper to consume over two GB of memory which leads to an OOM failure in the 32-bit depot_tools python, and the loss of all of the valuable output. This change modifies ExecLinkWrapper to process the output one line at a time, thus avoiding the OOM. I've tested that this handles the 1.1 GB of output which the previous version of this function failed on and I've visually confirmed that the output looks the same - no extraneous blank lines, for instance, when displaying warnings, errors, or 1.9 million lines of verbose output. I also verified that the script stays idle when waiting for output - blocking on .readline(). BUG=672182 Committed: https://crrev.com/5d0d1b0bf01acb3ebe3a5ef27ebf2f3180ab1d1d Cr-Commit-Position: refs/heads/master@{#437126} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5d0d1b0bf01acb3ebe3a5ef27ebf2f3180ab1d1d Cr-Commit-Position: refs/heads/master@{#437126}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2564893002/ by scottmg@chromium.org. The reason for reverting is: Reverting under suspicion for crbug.com/672841.. |
