https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.cc
File src/heap/identity-map.cc (right):
https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.cc#newcode17
src/heap/identity-map.cc:17: if (concurrent_)
heap_->relocation_mutex()->Lock();
On 2015/04/27 04:04:17, Benedikt Meurer wrote:
How about a simple helper class like this:
namespace {
class AutoLocker {
public:
AutoLocker(Heap* heap, bool concurrent) : heap_(heap),
concurrent_(concurent)
{
if (concurrent_) heap_->relocation_mutex()->Lock();
}
~AutoLocker() { if (concurrent_)
heap_->relocation_mutex()->Unlock(); }
private:
Heap* heap_;
bool concurrent_;
};
}
and then use it like this:
AutoLocker locker(heap_, concurrent_);
same below. That should make the code more robust.
Done.
I've made one in the Heap called OptionalRelocationLock. Now the mutex
doesn't have to be accessible outside.
https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h
File src/heap/identity-map.h (right):
https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h#newcode51
src/heap/identity-map.h:51: RawEntry Lookup(Handle<Object> handle) {
On 2015/04/27 04:04:18, Benedikt Meurer wrote:
Style nit: Since these methods are private, and all call sites are in
the .cc
file anyway, there's probably no reason to have them in the .h file.
Can you
move them to the .cc file as well?
Done.
https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h#newcode53
src/heap/identity-map.h:53: int index = LookupIndex(*handle);
On 2015/04/27 04:04:18, Benedikt Meurer wrote:
How about size_t here, and check for index < size_ instead of index >=
0?
I think int works better here, since we are using as an index, and
negative for out-of-bounds is a pretty clear indication.
https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h#newcode57
src/heap/identity-map.h:57: RawEntry Insert(Handle<Object> handle) {
On 2015/04/27 04:04:17, Benedikt Meurer wrote:
Style nit: Since these methods are private, and all call sites are in
the .cc
file anyway, there's probably no reason to have them in the .h file.
Can you
move them to the .cc file as well?
Done.
https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h#newcode64
src/heap/identity-map.h:64: int Hash(Object* address) {
On 2015/04/27 04:04:18, Benedikt Meurer wrote:
Style nit: Since these methods are private, and all call sites are in
the .cc
file anyway, there's probably no reason to have them in the .h file.
Can you
move them to the .cc file as well?
Done.
https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h#newcode68
src/heap/identity-map.h:68: return static_cast<int>((raw_address >> 11)
^ raw_address);
On 2015/04/27 04:04:17, Benedikt Meurer wrote:
Down casting to int looks wrong here. How about using uintptr_t here?
Or even
better use size_t?
size_t isn't any safer than int. As per below comment, both are
implementation-dependent, but this is a hash operation, so it doesn't
matter.
https://codereview.chromium.org/1105693002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
