LGTM. A few comments to address, but mainly nits.

https://chromiumcodereview.appspot.com/9235029/diff/1/src/heap.cc
File src/heap.cc (right):

https://chromiumcodereview.appspot.com/9235029/diff/1/src/heap.cc#newcode899
src/heap.cc:899: FlushNumberStringCache();
If it's intentional that the new heuristic for flushing is "always
flush", then I am fine with that. Please document that in the issue
tracker.

https://chromiumcodereview.appspot.com/9235029/diff/1/src/heap.cc#newcode2630
src/heap.cc:2630: // max_semispace_size_ == 512 KB =>
number_string_cache_size = 32.
This comment no longer is on sync with the computation below.

https://chromiumcodereview.appspot.com/9235029/diff/1/src/heap.cc#newcode2633
src/heap.cc:2633: number_string_cache_size =
Max(kInitialNumberStringCacheSize * 2,
The constant is multiplied with 2 twice. I think we can drop this
multiplication.

https://chromiumcodereview.appspot.com/9235029/diff/1/src/heap.cc#newcode2647
src/heap.cc:2647: if (maybe_obj->ToObject(&new_cache)) {
Can we add a comment that this is a best effort allocation and we ignore
failures here?

https://chromiumcodereview.appspot.com/9235029/diff/1/src/heap.h
File src/heap.h (right):

https://chromiumcodereview.appspot.com/9235029/diff/1/src/heap.h#newcode1804
src/heap.h:1804: // Initializes the number to string cache based on the
max semispace size.
This comment line should either be moved up two lines or be dropped
completely.

https://chromiumcodereview.appspot.com/9235029/

--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev

Reply via email to