Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1798243002:
Fix the block stack used by the bitstream cursor. (Closed)
|
Created:
4 years, 9 months ago by Karl Modified:
4 years, 9 months ago CC:
native-client-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-llvm.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionFix the block stack used by the bitstream cursor.
The previous implementation of the bitstream cursor copied global
abbreviations for each block into its own vector, and deleted the
vector when the vector went out of scope. Further aggrevating the
situation is that function blocks can have hundreds of global
abbreviations that need to be copied.
This results in over 500k calls to malloc during parsing. Since PNaCl
compiles using newlib, and uses multiple threads, this introduces a
noticeable slowdown.
This CL fixes this problem by refering to the collected global
abbreviations directly, thereby removing these abbreviation copies.
BUG=None
R=dschuff@chromium.org
Committed: https://chromium.googlesource.com/native_client/pnacl-llvm/+/a65844702e4a122c89225a94f8edf01941fb8e02
Patch Set 1 #
Total comments: 26
Patch Set 2 : Fix issues raised in CL. #
Total comments: 2
Patch Set 3 : Clean up issues with appendUnique and appendShared #Patch Set 4 : Fix nits. #
Total comments: 4
Patch Set 5 : Fix nits. #
Messages
Total messages: 15 (4 generated)
Description was changed from ========== Fix the block stack used by the bitstream cursor. The previous implementation of the bitstream cursor copied global abbreviations for each block into it's own vector, and deleted the vector when the vector went out of scope. Further aggrevating the situation is that function blocks can have hundreds of global abbreviations that need to be copied. This results in over 500k calls to malloc during parsing. Since PNaCl compiles using newlib, and uses multiple threads, this introduces a noticeable slowdown. This CL fixes this problem by refering to the collected global abbreviations directly, thereby removing these abbreviation copies. BUG=None ========== to ========== Fix the block stack used by the bitstream cursor. The previous implementation of the bitstream cursor copied global abbreviations for each block into it's own vector, and deleted the vector when the vector went out of scope. Further aggrevating the situation is that function blocks can have hundreds of global abbreviations that need to be copied. This results in over 500k calls to malloc during parsing. Since PNaCl compiles using newlib, and uses multiple threads, this introduces a noticeable slowdown. This CL fixes this problem by refering to the collected global abbreviations directly, thereby removing these abbreviation copies. BUG=None ==========
kschimpf@google.com changed reviewers: + dschuff@chromium.org, stichnot@chromium.org
Description was changed from ========== Fix the block stack used by the bitstream cursor. The previous implementation of the bitstream cursor copied global abbreviations for each block into it's own vector, and deleted the vector when the vector went out of scope. Further aggrevating the situation is that function blocks can have hundreds of global abbreviations that need to be copied. This results in over 500k calls to malloc during parsing. Since PNaCl compiles using newlib, and uses multiple threads, this introduces a noticeable slowdown. This CL fixes this problem by refering to the collected global abbreviations directly, thereby removing these abbreviation copies. BUG=None ========== to ========== Fix the block stack used by the bitstream cursor. The previous implementation of the bitstream cursor copied global abbreviations for each block into its own vector, and deleted the vector when the vector went out of scope. Further aggrevating the situation is that function blocks can have hundreds of global abbreviations that need to be copied. This results in over 500k calls to malloc during parsing. Since PNaCl compiles using newlib, and uses multiple threads, this introduces a noticeable slowdown. This CL fixes this problem by refering to the collected global abbreviations directly, thereby removing these abbreviation copies. BUG=None ==========
https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... File include/llvm/Bitcode/NaCl/NaClBitCodes.h (right): https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... include/llvm/Bitcode/NaCl/NaClBitCodes.h:25: #include <climits> Should includes be sorted? https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right): https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:60: static constexpr size_t DefaultAbbrevListSize = 12; same comment about constexpr https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:127: // Allow NaClBitstreamCursor to update, so that it can install reflow comment (I think) here and below https://codereview.chromium.org/1798243002/diff/1/lib/Bitcode/NaCl/Reader/NaC... File lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp (right): https://codereview.chromium.org/1798243002/diff/1/lib/Bitcode/NaCl/Reader/NaC... lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp:74: constexpr bool IsFixed = true; I think LLVM is still generally not using constexpr. https://codereview.chromium.org/1798243002/diff/1/lib/Bitcode/NaCl/Reader/NaC... lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp:373: // ReadAbbrevRecord installs as local abbreviation. Move it to s/as/a/ ? https://codereview.chromium.org/1798243002/diff/1/lib/Bitcode/NaCl/Reader/NaC... lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp:374: // the appropriate BlockInfo if the corresponding SetBID record reflow comment to 80-col
https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... File include/llvm/Bitcode/NaCl/NaClBitCodes.h (right): https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... include/llvm/Bitcode/NaCl/NaClBitCodes.h:89: TOP_LEVEL_BLOCKID = UINT_MAX Does it matter that this block will be considered an application block id by NaClBitcodeBlockDist.cpp and NaClBitcodeCodeDist.cpp? https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right): https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:79: Abbrevs.push_back(Abbv); I don't really understand why you have to call addRef() in the other push_back but not here? https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:81: void push_back(const AbbrevList &NewAbbrevs) { It seems a little weird that this is also called push_back, especially since there is no analog in the std containers. Maybe it should be called extend, or something that indicates its intent better? (but probably not insert, unless you want to implement the same semantics as std::vector::insert but that's probably overkill). https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:106: class BlockInfo { does a BLOCKINFO_BLOCK have information other than abbreviations that isn't modeled here? If so, maybe this class should be called BlockAbbrevInfo or something like that? https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:122: void destroy() { Abbrevs.clear(); } Is destroy() used anywhere? https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:139: // True if ReadBlockInfoBlock has been called. Please fix the bug in English that the past and present tense of "read" are the same. https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:702: if (BlockScope.empty()) return true; extra space?
https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... File include/llvm/Bitcode/NaCl/NaClBitCodes.h (right): https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... include/llvm/Bitcode/NaCl/NaClBitCodes.h:25: #include <climits> On 2016/03/14 22:46:49, stichnot wrote: > Should includes be sorted? This code was (originally) copied from LLVM, and it wasn't sorted. From what I can tell, they don't do this in general. The goal with these files it to create a little churn as possible, so that it is easier to incorporate upstream changes in LLVM. https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... include/llvm/Bitcode/NaCl/NaClBitCodes.h:89: TOP_LEVEL_BLOCKID = UINT_MAX On 2016/03/15 17:47:04, Derek Schuff wrote: > Does it matter that this block will be considered an application block id by > NaClBitcodeBlockDist.cpp and NaClBitcodeCodeDist.cpp? Yes, you are right. It does raise TOP_LEVEL_BLOCKID to an application block. Moving this to value 7, assuming we will be less likely to get affected by LLVM changes. https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right): https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:60: static constexpr size_t DefaultAbbrevListSize = 12; On 2016/03/14 22:46:49, stichnot wrote: > same comment about constexpr changed to "const". https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:79: Abbrevs.push_back(Abbv); On 2016/03/15 17:47:04, Derek Schuff wrote: > I don't really understand why you have to call addRef() in the other push_back > but not here? This is part of the confusing usage of reference counts, because they don't use a container class to maintain the reference counts. As a result, the user's must be careful and follow the rules that when an abbreviation is first allocated, the reference count is 1. To try and clarify this, replaced this with methods appendUnique and appendShare. appendUnique is used when the reference count has already been incremented (i.e. like a unique_ptr, it owns the abbreviation and the method takes ownership). appendShare is used when the reference count hasn't yet been incremented (i.e. like a shared_ptr, where the caller continue to have ownership, and the method takes ownership as well). https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:81: void push_back(const AbbrevList &NewAbbrevs) { On 2016/03/15 17:47:04, Derek Schuff wrote: > It seems a little weird that this is also called push_back, especially since > there is no analog in the std containers. Maybe it should be called extend, or > something that indicates its intent better? (but probably not insert, unless you > want to implement the same semantics as std::vector::insert but that's probably > overkill). Changed to appendList, to better clarify. https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:106: class BlockInfo { On 2016/03/15 17:47:04, Derek Schuff wrote: > does a BLOCKINFO_BLOCK have information other than abbreviations that isn't > modeled here? If so, maybe this class should be called BlockAbbrevInfo or > something like that? For LLVM, there is more information. for PNaCl bitcode files, no other information is stored in it. We never changed the name because: 1) We wanted to stay close to LLVM code for bitstream reading. 2) A number of other files refer to BlockInfo, and would have to be edited to change the name. https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:122: void destroy() { Abbrevs.clear(); } On 2016/03/15 17:47:04, Derek Schuff wrote: > Is destroy() used anywhere? Good catch. This clear has already been moved to AbbrevList, and doesn't have to be done. Removing destroy() and simplifying the destructor. https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:127: // Allow NaClBitstreamCursor to update, so that it can install On 2016/03/14 22:46:49, stichnot wrote: > reflow comment (I think) > > here and below Done by removing need for friend class. https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:139: // True if ReadBlockInfoBlock has been called. On 2016/03/15 17:47:04, Derek Schuff wrote: > Please fix the bug in English that the past and present tense of "read" are the > same. https://codereview.chromium.org/1798243002/diff/1/include/llvm/Bitcode/NaCl/N... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:702: if (BlockScope.empty()) return true; On 2016/03/15 17:47:04, Derek Schuff wrote: > extra space? Done. https://codereview.chromium.org/1798243002/diff/1/lib/Bitcode/NaCl/Reader/NaC... File lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp (right): https://codereview.chromium.org/1798243002/diff/1/lib/Bitcode/NaCl/Reader/NaC... lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp:74: constexpr bool IsFixed = true; On 2016/03/14 22:46:49, stichnot wrote: > I think LLVM is still generally not using constexpr. Done. https://codereview.chromium.org/1798243002/diff/1/lib/Bitcode/NaCl/Reader/NaC... lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp:373: // ReadAbbrevRecord installs as local abbreviation. Move it to On 2016/03/14 22:46:49, stichnot wrote: > s/as/a/ ? Done. https://codereview.chromium.org/1798243002/diff/1/lib/Bitcode/NaCl/Reader/NaC... lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp:374: // the appropriate BlockInfo if the corresponding SetBID record On 2016/03/14 22:46:49, stichnot wrote: > reflow comment to 80-col Done.
https://codereview.chromium.org/1798243002/diff/20001/include/llvm/Bitcode/Na... File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right): https://codereview.chromium.org/1798243002/diff/20001/include/llvm/Bitcode/Na... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:80: void appendUnique(NaClBitCodeAbbrev *Abbv) { Does appendUnique mean that nothing else will ever get a reference to it? If so then this naming is fine. If this is just the first reference (and we just have to avoid incrementing the refcount because it starts at 1) then maybe we could call it appendFirst, or something?
https://codereview.chromium.org/1798243002/diff/20001/include/llvm/Bitcode/Na... File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right): https://codereview.chromium.org/1798243002/diff/20001/include/llvm/Bitcode/Na... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:80: void appendUnique(NaClBitCodeAbbrev *Abbv) { On 2016/03/17 16:20:17, Derek Schuff wrote: > Does appendUnique mean that nothing else will ever get a reference to it? If so > then this naming is fine. If this is just the first reference (and we just have > to avoid incrementing the refcount because it starts at 1) then maybe we could > call it appendFirst, or something? Simplified the class by adding a "appendCreate" method that creates a new abbreviation and appends it to the list. Then, we only need append() and appendList(), and the user is no longer exposed to reference counts.
Ping?
https://codereview.chromium.org/1798243002/diff/60001/include/llvm/Bitcode/Na... File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right): https://codereview.chromium.org/1798243002/diff/60001/include/llvm/Bitcode/Na... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:95: // Returns last abbreviation on list, and returns an unique reference (i.e. Does this comment still apply? also an -> a https://codereview.chromium.org/1798243002/diff/60001/include/llvm/Bitcode/Na... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:128: #if 0 wat
https://codereview.chromium.org/1798243002/diff/60001/include/llvm/Bitcode/Na... File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right): https://codereview.chromium.org/1798243002/diff/60001/include/llvm/Bitcode/Na... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:95: // Returns last abbreviation on list, and returns an unique reference (i.e. On 2016/03/21 17:54:54, Derek Schuff wrote: > Does this comment still apply? > also an -> a Removed reference to unique reference. https://codereview.chromium.org/1798243002/diff/60001/include/llvm/Bitcode/Na... include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:128: #if 0 On 2016/03/21 17:54:54, Derek Schuff wrote: > wat Removed code.
lgtm
Description was changed from ========== Fix the block stack used by the bitstream cursor. The previous implementation of the bitstream cursor copied global abbreviations for each block into its own vector, and deleted the vector when the vector went out of scope. Further aggrevating the situation is that function blocks can have hundreds of global abbreviations that need to be copied. This results in over 500k calls to malloc during parsing. Since PNaCl compiles using newlib, and uses multiple threads, this introduces a noticeable slowdown. This CL fixes this problem by refering to the collected global abbreviations directly, thereby removing these abbreviation copies. BUG=None ========== to ========== Fix the block stack used by the bitstream cursor. The previous implementation of the bitstream cursor copied global abbreviations for each block into its own vector, and deleted the vector when the vector went out of scope. Further aggrevating the situation is that function blocks can have hundreds of global abbreviations that need to be copied. This results in over 500k calls to malloc during parsing. Since PNaCl compiles using newlib, and uses multiple threads, this introduces a noticeable slowdown. This CL fixes this problem by refering to the collected global abbreviations directly, thereby removing these abbreviation copies. BUG=None R=dschuff@chromium.org Committed: https://chromium.googlesource.com/native_client/pnacl-llvm/+/a65844702e4a122c... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as a65844702e4a122c89225a94f8edf01941fb8e02 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
