I don't think we should mention what specific JavaScript code an optimization is targeted at (packer.js) in the source comments, but state why it is a good idea in more general terms.
I have just done StringAdd in native code (with the changed string representation separating length and hash), so we need to figure out whether to handle two char strings in the runtime system still or do hash calculation and symbol table lookup (perhaps just a single probe) in native code if to does not get to complicated and error phrone. http://codereview.chromium.org/409007/diff/1/2 File src/heap.cc (right): http://codereview.chromium.org/409007/diff/1/2#newcode1784 Line 1784: String* symbol; Please add a comment on why digits only will not work here. http://codereview.chromium.org/409007/diff/1/2#newcode1785 Line 1785: if ((c1 - '0' > '9' - '0' || Why c1 - '0' > '9' - '0' instead of c1 > '9'? http://codereview.chromium.org/409007/diff/1/2#newcode1878 Line 1878: } else if (length == 2) { Refactor duplicated code. http://codereview.chromium.org/409007/diff/1/7 File src/heap.h (right): http://codereview.chromium.org/409007/diff/1/7#newcode634 Line 634: static bool LookupTwoCharsIfExists(String* str, String** symbol); LookupTwoCharsIfExists -> LookupTwoCharsSymbolIfExists to match the other symbol table lookup functions. Comment that it does not work for digits. http://codereview.chromium.org/409007/diff/1/6 File src/objects-inl.h (right): http://codereview.chromium.org/409007/diff/1/6#newcode3113 Line 3113: const int kArraySizeThatFitsComfortablyInNewSpace = 128; This does not take the calculation required_size + (required_size >> 3) in Expand into account... http://codereview.chromium.org/409007/diff/1/6#newcode3114 Line 3114: if (required_size < kArraySizeThatFitsComfortablyInNewSpace) { Maybe combine these two if's into one if condition. Also make a note that "Expand" will always allocate a new backing store even if it is not expanding. http://codereview.chromium.org/409007/diff/1/3 File src/objects.cc (right): http://codereview.chromium.org/409007/diff/1/3#newcode420 Line 420: // constantly reallocating them. This code is duplicated below, please refactor. http://codereview.chromium.org/409007/diff/1/3#newcode7350 Line 7350: : c1_(c1), c2_(c2) { How about ASSERT(c1 > '9' || c2 > '9') here? I see that the ASSERT_EQ below will catch it, but it might be a bit obscure why. http://codereview.chromium.org/409007/diff/1/4 File src/runtime.cc (right): http://codereview.chromium.org/409007/diff/1/4#newcode2361 Line 2361: if (from->IsSmi() && to->IsSmi()) { Maybe add a comment to this fast case. http://codereview.chromium.org/409007/diff/1/4#newcode2364 Line 2364: Please move the declaration of start and end before the if and the following four lines after the else block to avoid commin code in if and else block. http://codereview.chromium.org/409007 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
