Add a write buffer, aligned so that a single bit tells us whether it has...

Add a write buffer, aligned so that a single bit tells us whether it has
overflowed.  Not yet used.

[Vyacheslav Egorov (Chromium), 2010-12-14 22:40:14]

wrong branch.

LGTM (though I am bit confused by some seemingly unrelated changes here and
there)

http://codereview.chromium.org/5826004/diff/1/src/assembler.h
File src/assembler.h (right):

http://codereview.chromium.org/5826004/diff/1/src/assembler.h#newcode537
src/assembler.h:537: // Write barrier.
barrier -> buffer.

http://codereview.chromium.org/5826004/diff/1/src/write-buffer.cc
File src/write-buffer.cc (right):

http://codereview.chromium.org/5826004/diff/1/src/write-buffer.cc#newcode39
src/write-buffer.cc:39: virtual_memory_ = new VirtualMemory(kWriteBufferSize *
3);
This allocation looks OK for single buffer but I am not sure that it is OK for
multiple buffers (if we decide to have many of them).

On Windows which has allocation granularity 64k this is going to waste 98k of
address space.

Even on Linux it might be beneficial to return unused parts of this memory back
to OS.

I see no obvious benefit for single bit vs. several bits on x86 (though benefit
is obvious on ARM): mask = (kWriteBufferOverflowBit - 1), alignent =
kWriteBufferOverflowBit.

http://codereview.chromium.org/5826004/diff/1/src/write-buffer.h
File src/write-buffer.h (right):

http://codereview.chromium.org/5826004/diff/1/src/write-buffer.h#newcode39
src/write-buffer.h:39: class WriteBuffer : public AllStatic {
Consider adding a small comment about the role of this class.

http://codereview.chromium.org/5826004/diff/1/src/write-buffer.h#newcode47
src/write-buffer.h:47: static const int kWriteBufferOverflowBit = 1 << 15;
15 and 32 are connected.

consider expressing this connection through introduction of a constant

[Erik Corry, 2010-12-15 08:00:13]

I will commit to the correct branch.

http://codereview.chromium.org/5826004/diff/1/src/assembler.h
File src/assembler.h (right):

http://codereview.chromium.org/5826004/diff/1/src/assembler.h#newcode537
src/assembler.h:537: // Write barrier.
On 2010/12/14 22:40:14, Vyacheslav Egorov wrote:
> barrier -> buffer.

The write barrier is implemented using a write buffer.  It would be redundant to
have 'write buffer' in the comment would be redundant.

http://codereview.chromium.org/5826004/diff/1/src/write-buffer.cc
File src/write-buffer.cc (right):

http://codereview.chromium.org/5826004/diff/1/src/write-buffer.cc#newcode39
src/write-buffer.cc:39: virtual_memory_ = new VirtualMemory(kWriteBufferSize *
3);
On 2010/12/14 22:40:14, Vyacheslav Egorov wrote:
> This allocation looks OK for single buffer but I am not sure that it is OK for
> multiple buffers (if we decide to have many of them).

I'll burn that bridge when I come to it.

> 
> On Windows which has allocation granularity 64k this is going to waste 98k of
> address space.

I think that 0.0025% of the address space is worth spending on my awsum idea. 
The new space will use 160 times as much.

> Even on Linux it might be beneficial to return unused parts of this memory
back
> to OS.



> I see no obvious benefit for single bit vs. several bits on x86 (though
benefit
> is obvious on ARM): mask = (kWriteBufferOverflowBit - 1), alignent =
> kWriteBufferOverflowBit.

It's definitely more compact on ARM and I like that.

http://codereview.chromium.org/5826004/diff/1/src/write-buffer.h
File src/write-buffer.h (right):

http://codereview.chromium.org/5826004/diff/1/src/write-buffer.h#newcode39
src/write-buffer.h:39: class WriteBuffer : public AllStatic {
On 2010/12/14 22:40:14, Vyacheslav Egorov wrote:
> Consider adding a small comment about the role of this class.

Done.

http://codereview.chromium.org/5826004/diff/1/src/write-buffer.h#newcode47
src/write-buffer.h:47: static const int kWriteBufferOverflowBit = 1 << 15;
On 2010/12/14 22:40:14, Vyacheslav Egorov wrote:
> 15 and 32 are connected.
> 
> consider expressing this connection through introduction of a constant

Done.