Seems much better, and safer.
I would consider creating all caches from C++ code during bootstrap (perhaps
have an enumeration of caches, so you know the number), instead of creating
them
in toplevel JSCode in natives files (which is also run during bootstrap
anyway).
That would make, e.g., the test for an undefined cache unnecessary, and you
can
check the literal smi cache id more strictly.
You will need to do some native X64 and ARM calls, possibly just to the C++
runtime functions (or it won't compile).
http://codereview.chromium.org/1563005/diff/9012/25002
File src/contexts.h (right):
http://codereview.chromium.org/1563005/diff/9012/25002#newcode85
src/contexts.h:85: V(NATIVE_CACHES_INDEX, Object, native_caches) \
The name seems like we are caching natives (q.v. FUNCTION_CACHE_INDEX).
How about JSFUNCTION_RESULT_CACHES_INDEX?
http://codereview.chromium.org/1563005/diff/9012/25005
File src/heap.cc (right):
http://codereview.chromium.org/1563005/diff/9012/25005#newcode1670
src/heap.cc:1670: // Allocate cache for single character strings.
-> ... for single character ASCII strings.
http://codereview.chromium.org/1563005/diff/9012/25007
File src/ia32/codegen-ia32.cc (right):
http://codereview.chromium.org/1563005/diff/9012/25007#newcode6541
src/ia32/codegen-ia32.cc:6541: int cache_id =
Smi::cast(*(args->at(0)->AsLiteral()->handle()))->value();
Would be nice to have an assert that args->at(0) is in fact a literal,
just to crash with a better error message if we write bad code.
http://codereview.chromium.org/1563005/diff/9012/25007#newcode6550
src/ia32/codegen-ia32.cc:6550: if (o->IsUndefined()) {
Why have undefined fields in the FixedArray, instead of just a shorter
array? Then you could just do a check on index vs. length (which is
asserted by FixedArray::get in debug mode, but otherwise not checked).
http://codereview.chromium.org/1563005/diff/9012/25009
File src/runtime.cc (right):
http://codereview.chromium.org/1563005/diff/9012/25009#newcode9984
src/runtime.cc:9984: ASSERT(args.length() == 3);
Is there an advantage to creating new caches at runtime, instead of
creating them all at boot-strap time?
http://codereview.chromium.org/1563005/diff/9012/25009#newcode9992
src/runtime.cc:9992: Handle<JSFunction> factory(factory_obj);
You can use CONVERT_ARG_CHECKED(JSFunction, factory, 2) to create a
Handle<JSFunction> directly backed by the parameter on the stack,
instead of creating a new handle from a JSFunction*.
http://codereview.chromium.org/1563005/diff/9012/25009#newcode9994
src/runtime.cc:9994: int cache_id = cache_id_obj->value();
There's also CONVERT_SMI_CHECKED(cache_id, args[0]) that does what you
want in one line.
http://codereview.chromium.org/1563005/diff/9012/25009#newcode10022
src/runtime.cc:10022: cache->set(kCacheSizeIndex,
Smi::FromInt(kEntriesIndex));
kEntriesIndex->size
http://codereview.chromium.org/1563005/diff/9012/25009#newcode10025
src/runtime.cc:10025: }
If the cache already exists, but have a different size, is it ok to
ignore this call (just make a comment saying that it is, because we
should only initialize once).
http://codereview.chromium.org/1563005/diff/9012/25009#newcode10065
src/runtime.cc:10065: static Object* Runtime_GetFromCache(Arguments
args) {
This function is never called from JS code, but only as the slow case of
%_GetFromCache, correct?
In that case, we don't have to test the arguments as throughly, but it's
worth to make a comment about it, so we don't wonder why we don't check
the indices against the length of the cache array.
http://codereview.chromium.org/1563005/diff/9012/25009#newcode10079
src/runtime.cc:10079: // Search the cache in less recently used order.
(Well, possibly LRU, as long as we don't get a hit - which would move
the finger.)
http://codereview.chromium.org/1563005/diff/9012/25011
File src/string.js (right):
http://codereview.chromium.org/1563005/diff/9012/25011#newcode519
src/string.js:519: %NewCache(0, 16, function(re) { return new
$RegExp(re); });
Create constants in macros.py, e.g., STRING_SEARCH_REGEXP_CACHE_ID = 0
(or something slightly shorter perhaps :)
This cache is created during bootstrapping (when builtins files are
run), so you could also just create the FixedArray from C++-code.
http://codereview.chromium.org/1563005/diff/9012/25011#newcode519
src/string.js:519: %NewCache(0, 16, function(re) { return new
$RegExp(re); });
Could you just use $RegExp as the function (it works as a constructor,
even if not called using "new")?
http://codereview.chromium.org/1563005/diff/9012/25012
File test/mjsunit/fuzz-natives.js (right):
http://codereview.chromium.org/1563005/diff/9012/25012#newcode164
test/mjsunit/fuzz-natives.js:164: // additional checks.
Also: It's checked that all call points are using a literal smi as first
argument (perhaps also check that the second argument is a reasonably
small smi literal), and dying violently in the case that they are not is
expected behavior.
http://codereview.chromium.org/1563005/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
To unsubscribe, reply using "remove me" as the subject.
