Issue 1838203002: Modify NaCl bitstream reader to allow parallel parses.

Issue 1838203002: Modify NaCl bitstream reader to allow parallel parses. (Closed)

Created:
4 years, 8 months ago by Karl

Modified:
4 years, 8 months ago

Reviewers:
Jim Stichnoth, Derek Schuff, John

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.

More Reviews

Description

Modify NaCl bitstream reader to allow parallel parses. Fixes class NaClBitstreamReader to allow parallel parses. In particular, it makes the global abbreviations a shared object, so that it can be shared by other bitstream readers. To make the overhead small, the lookup map of the global abbreviations is initialized up front with all known block ids, rather than incrementally updating. This reduces the locking considerably in that the block lookup no longer needs a lock. Finally, it adds a constructor to class NaClBitstreamReader that allows one to construct a new (parallel) bitstream reader from an existing bistream reader. This code assumes that parallel parsing is done with 1 driver parse thread, and n helper parse threads to parse function blocks. The driver thread is responsible for parsing the memory object, and spawning off parallel parses for each function block. To guarantee correctness (since memory objects may be dynamically filled), the driver parser should build a buffer and copy the corresponding bytes of the function block into that buffer, before spawning off the parallel parser to parse it. BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4363 R=jpp@chromium.org, stichnot@chromium.org Committed: https://chromium.googlesource.com/native_client/pnacl-llvm/+/9f7c780be51eed366c10a97a2cf41b5d7da8a533

Patch Set 1 #

Total comments: 16

Patch Set 2 : Fixed nits. #

Total comments: 4

Patch Set 3 : Fix nits. #

Created: 4 years, 8 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+161 lines, -43 lines)			Patch
	M	include/llvm/Bitcode/NaCl/NaClBitstreamReader.h	View	1 2	7 chunks	+122 lines, -34 lines	0 comments	Download
	M	lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp	View	1 2	4 chunks	+39 lines, -9 lines	0 comments	Download

Messages

Total messages: 8 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Karl

Description was changed from ========== Modify NaCl bitstream reader to allow parallel parses. Fixes class ...

4 years, 8 months ago (2016-03-29 15:55:39 UTC) #1

Description was changed from

==========
Modify NaCl bitstream reader to allow parallel parses.

Fixes class NaClBitstreamReader to allow parallel parses. In
particular, it makes the global abbreviations a shared object, so that
it can be shared by other bitstream readers.

To make the overhead small, the lookup map of the global abbreviations
is initialized up front with all known block ids, rather than
incrementally updating. This reduces the locking considerably in that
the block lookup no longer needs a lock.

Finally, it adds a constructor to class NaClBitstreamReader that
allows one to construct a new (parallel) bitstream reader from an
existing bistream reader.

This code assumes that parallel parsing is done with 1 driver parse
thread, and n helper parse threads to parse function blocks. The
driver thread is responsible for parsing the memory object, and
spawning off parallel parses for each function block. To guarantee
correctness (since memory objects may be dynamically filled), the
driver parser should build a buffer and copy the corresponding bytes
of the function block into that buffer, before spawning off the
parallel parser to parse it.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4363
==========

to

==========
Modify NaCl bitstream reader to allow parallel parses.

Fixes class NaClBitstreamReader to allow parallel parses. In
particular, it makes the global abbreviations a shared object, so that
it can be shared by other bitstream readers.

To make the overhead small, the lookup map of the global abbreviations
is initialized up front with all known block ids, rather than
incrementally updating. This reduces the locking considerably in that
the block lookup no longer needs a lock.

Finally, it adds a constructor to class NaClBitstreamReader that
allows one to construct a new (parallel) bitstream reader from an
existing bistream reader.

This code assumes that parallel parsing is done with 1 driver parse
thread, and n helper parse threads to parse function blocks. The
driver thread is responsible for parsing the memory object, and
spawning off parallel parses for each function block. To guarantee
correctness (since memory objects may be dynamically filled), the
driver parser should build a buffer and copy the corresponding bytes
of the function block into that buffer, before spawning off the
parallel parser to parse it.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4363
==========

John

lgtm https://codereview.chromium.org/1838203002/diff/1/include/llvm/Bitcode/NaCl/NaClBitstreamReader.h File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right): https://codereview.chromium.org/1838203002/diff/1/include/llvm/Bitcode/NaCl/NaClBitstreamReader.h#newcode26 include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:26: #include <unordered_map> alphabetize? https://codereview.chromium.org/1838203002/diff/1/include/llvm/Bitcode/NaCl/NaClBitstreamReader.h#newcode156 include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:156: return std::shared_ptr<BlockInfoRecordsMap>(new BlockInfoRecordsMap()); ...

4 years, 8 months ago (2016-03-29 16:27:11 UTC) #4

Jim Stichnoth

lgtm https://codereview.chromium.org/1838203002/diff/20001/include/llvm/Bitcode/NaCl/NaClBitstreamReader.h File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right): https://codereview.chromium.org/1838203002/diff/20001/include/llvm/Bitcode/NaCl/NaClBitstreamReader.h#newcode136 include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:136: // Holds the global abbreviations in the BlockInfo ...

4 years, 8 months ago (2016-03-29 19:47:18 UTC) #5

Karl

Description was changed from ========== Modify NaCl bitstream reader to allow parallel parses. Fixes class ...

4 years, 8 months ago (2016-03-29 20:10:06 UTC) #6

Description was changed from

==========
Modify NaCl bitstream reader to allow parallel parses.

Fixes class NaClBitstreamReader to allow parallel parses. In
particular, it makes the global abbreviations a shared object, so that
it can be shared by other bitstream readers.

To make the overhead small, the lookup map of the global abbreviations
is initialized up front with all known block ids, rather than
incrementally updating. This reduces the locking considerably in that
the block lookup no longer needs a lock.

Finally, it adds a constructor to class NaClBitstreamReader that
allows one to construct a new (parallel) bitstream reader from an
existing bistream reader.

This code assumes that parallel parsing is done with 1 driver parse
thread, and n helper parse threads to parse function blocks. The
driver thread is responsible for parsing the memory object, and
spawning off parallel parses for each function block. To guarantee
correctness (since memory objects may be dynamically filled), the
driver parser should build a buffer and copy the corresponding bytes
of the function block into that buffer, before spawning off the
parallel parser to parse it.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4363
==========

to

==========
Modify NaCl bitstream reader to allow parallel parses.

Fixes class NaClBitstreamReader to allow parallel parses. In
particular, it makes the global abbreviations a shared object, so that
it can be shared by other bitstream readers.

To make the overhead small, the lookup map of the global abbreviations
is initialized up front with all known block ids, rather than
incrementally updating. This reduces the locking considerably in that
the block lookup no longer needs a lock.

Finally, it adds a constructor to class NaClBitstreamReader that
allows one to construct a new (parallel) bitstream reader from an
existing bistream reader.

This code assumes that parallel parsing is done with 1 driver parse
thread, and n helper parse threads to parse function blocks. The
driver thread is responsible for parsing the memory object, and
spawning off parallel parses for each function block. To guarantee
correctness (since memory objects may be dynamically filled), the
driver parser should build a buffer and copy the corresponding bytes
of the function block into that buffer, before spawning off the
parallel parser to parse it.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4363
R=jpp@chromium.org, stichnot@chromium.org

Committed:
https://chromium.googlesource.com/native_client/pnacl-llvm/+/9f7c780be51eed36...
==========

Karl

Committed patchset #3 (id:40001) manually as 9f7c780be51eed366c10a97a2cf41b5d7da8a533 (presubmit successful).

4 years, 8 months ago (2016-03-29 20:10:07 UTC) #7

Karl

4 years, 8 months ago (2016-03-29 20:10:47 UTC) #8

Message was sent while issue was closed.

https://codereview.chromium.org/1838203002/diff/1/include/llvm/Bitcode/NaCl/N...
File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right):

https://codereview.chromium.org/1838203002/diff/1/include/llvm/Bitcode/NaCl/N...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:26: #include <unordered_map>
On 2016/03/29 16:27:10, John wrote:
> alphabetize?

Done.

https://codereview.chromium.org/1838203002/diff/1/include/llvm/Bitcode/NaCl/N...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:156: return
std::shared_ptr<BlockInfoRecordsMap>(new BlockInfoRecordsMap());
On 2016/03/29 16:27:10, John wrote:
> perhaps you could use std::make_shared? you'll probably need this:
>
http://stackoverflow.com/questions/8147027/how-do-i-call-stdmake-shared-on-a-...

I followed the advice of "Effective Modern C++" by making the constructor
private and adding a "create()" method. Leaving as is.

https://codereview.chromium.org/1838203002/diff/1/include/llvm/Bitcode/NaCl/N...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:160: bool isFrozen() {
On 2016/03/29 16:27:10, John wrote:
> can this method be marked const?

Done.

https://codereview.chromium.org/1838203002/diff/1/include/llvm/Bitcode/NaCl/N...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:164: // Returns false if already
frozen.
On 2016/03/29 16:27:10, John wrote:
> returns true if already frozen, no?

Good catch. Forgot to update the comment when I changed the implementation.
Fixing.

https://codereview.chromium.org/1838203002/diff/1/include/llvm/Bitcode/NaCl/N...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:169: BlockInfo
&getBlockInfo(unsigned BlockID) {
On 2016/03/29 16:27:10, John wrote:
> can this method be marked const?

Unfortunately no. The way that the code updates the global abbreviations does
not allow it. Leaving as is.

https://codereview.chromium.org/1838203002/diff/1/include/llvm/Bitcode/NaCl/N...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:170: InfosMap::iterator Pos =
Infos.find(BlockID);
On 2016/03/29 16:27:10, John wrote:
> auto? I believe this is one of the few cases where auto is allowed in the llvm
> style guide...

Done.

https://codereview.chromium.org/1838203002/diff/1/lib/Bitcode/NaCl/Reader/NaC...
File lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp (right):

https://codereview.chromium.org/1838203002/diff/1/lib/Bitcode/NaCl/Reader/NaC...
lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp:346: } // end of anonymous
namespace
On 2016/03/29 16:27:10, John wrote:
> is this annotation part of the llvm coding style?

There is a similar comment at the beginning of the file for other anonymous
namespace definitions. Assuming Ok.

https://codereview.chromium.org/1838203002/diff/1/lib/Bitcode/NaCl/Reader/NaC...
lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp:350: for (size_t i = 0; i <
array_lengthof(ValidBlockIDs); ++i) {
On 2016/03/29 16:27:10, John wrote:
> you can use for each loops for static array !!!! (I just found that out)
> 
> see if this works:
> 
> for (size_t BlockID : ValidBlockIDs) {}

Done.

https://codereview.chromium.org/1838203002/diff/20001/include/llvm/Bitcode/Na...
File include/llvm/Bitcode/NaCl/NaClBitstreamReader.h (right):

https://codereview.chromium.org/1838203002/diff/20001/include/llvm/Bitcode/Na...
include/llvm/Bitcode/NaCl/NaClBitstreamReader.h:136: // Holds the global
abbreviations in the BlockInfo block of the bitcode
On 2016/03/29 19:47:18, stichnot wrote:
> reflow comment?

Done.

https://codereview.chromium.org/1838203002/diff/20001/lib/Bitcode/NaCl/Reader...
File lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp (right):

https://codereview.chromium.org/1838203002/diff/20001/lib/Bitcode/NaCl/Reader...
lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp:344: const size_t
InfosBucketSize = 23;
On 2016/03/29 19:47:18, stichnot wrote:
> Can you explain this magic constant?

Not worth it. Removing the magic.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages