Lasse, thanks a lot for comments and suggestions. I am starting on ports to ARM and x64.
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) \ On 2010/04/13 09:30:14, Lasse Reichstein wrote:
The name seems like we are caching natives (q.v.
FUNCTION_CACHE_INDEX). How
about JSFUNCTION_RESULT_CACHES_INDEX?
Sure, I like it better as well. 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. On 2010/04/13 09:30:14, Lasse Reichstein wrote:
-> ... for single character ASCII strings.
Done. 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(); On 2010/04/13 09:30:14, Lasse Reichstein wrote:
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.
Done. http://codereview.chromium.org/1563005/diff/9012/25007#newcode6550 src/ia32/codegen-ia32.cc:6550: if (o->IsUndefined()) { On 2010/04/13 09:30:14, Lasse Reichstein wrote:
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).
Tried to play safe. Imagine someone who by mistake puts wrong cache id. But I think that with your idea of creation of caches in C++, it is not needed anymore. 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); On 2010/04/13 09:30:14, Lasse Reichstein wrote:
Is there an advantage to creating new caches at runtime, instead of
creating
them all at boot-strap time?
I don't think so and really like your idea. Reimplementing with this approach. http://codereview.chromium.org/1563005/diff/9012/25009#newcode9992 src/runtime.cc:9992: Handle<JSFunction> factory(factory_obj); On 2010/04/13 09:30:14, Lasse Reichstein wrote:
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*.
Thanks, done. http://codereview.chromium.org/1563005/diff/9012/25009#newcode9994 src/runtime.cc:9994: int cache_id = cache_id_obj->value(); On 2010/04/13 09:30:14, Lasse Reichstein wrote:
There's also CONVERT_SMI_CHECKED(cache_id, args[0]) that does what you
want in
one line.
Done. http://codereview.chromium.org/1563005/diff/9012/25009#newcode10022 src/runtime.cc:10022: cache->set(kCacheSizeIndex, Smi::FromInt(kEntriesIndex)); On 2010/04/13 09:30:14, Lasse Reichstein wrote:
kEntriesIndex->size
sorry, do you suggest to change 2nd arg? if yes, I am not sure---caches are originally empty and thus it should be kEntriesIndex. http://codereview.chromium.org/1563005/diff/9012/25009#newcode10025 src/runtime.cc:10025: } On 2010/04/13 09:30:14, Lasse Reichstein wrote:
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).
It should be obsolete by now when it's not exposed in JS. http://codereview.chromium.org/1563005/diff/9012/25009#newcode10065 src/runtime.cc:10065: static Object* Runtime_GetFromCache(Arguments args) { On 2010/04/13 09:30:14, Lasse Reichstein wrote:
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.
Sure. Eventually it should all be in codegens. http://codereview.chromium.org/1563005/diff/9012/25009#newcode10079 src/runtime.cc:10079: // Search the cache in less recently used order. On 2010/04/13 09:30:14, Lasse Reichstein wrote:
(Well, possibly LRU, as long as we don't get a hit - which would move
the
finger.)
Do you want me to change wording? 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); }); On 2010/04/13 09:30:14, Lasse Reichstein wrote:
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.
Cool idea, thanks. http://codereview.chromium.org/1563005/diff/9012/25011#newcode519 src/string.js:519: %NewCache(0, 16, function(re) { return new $RegExp(re); }); On 2010/04/13 09:30:14, Lasse Reichstein wrote:
Could you just use $RegExp as the function (it works as a constructor,
even if
not called using "new")?
Sure. Done. For some reason I couldn't use $RegExp, had to revert to global.RegExp, any ideas why? 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. On 2010/04/13 09:30:14, Lasse Reichstein wrote:
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.
Is this wording good enough? 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.
