We ran compile-octane.js with a large number of iterations and code caching disabled. We didn’t single out any scripts, so mandrel might be interesting to look at like a single benchmark payload, yes.
/M On 09 Oct 2014, at 18:25, Hannes Wallnoefer <[email protected]> wrote: > Hi Andreas, > > This looks good. Some minor javadoc nitpicks: > > - First and last @param for ParserContextFunctionNode constructor do not exist > - Your @param tags often miss a description > - I think the proper way to link to an enum value is #VALUE, e.g. {@link > CompilationState#PARSED} in FunctionNode > > Apart from these minor issues the patch looks good. > > Out of curiosity, what scripts did you use for performance testing? Mandreel > might be interesting. > > Hannes > > Am 2014-10-08 um 11:13 schrieb Andreas Gabrielsson: >> Now all of Attila's comments should be fixed. I have also removed some >> public setters that were not needed anymore. >> >> Me and Marcus run a few performance tests and they passed, there were >> basically no difference. >> >> New webrev is at: http://cr.openjdk.java.net/~lagergren/8012518.5/ >> >> Andreas >> >> On 2014-09-16 16:06, Andreas Gabrielsson wrote: >>> I have fixed the thing you list below, thank you. >>> New webrev uploaded at: http://cr.openjdk.java.net/~lagergren/8012518.3/ >>> On 2014-09-16 10:50, Attila Szegedi wrote: >>>> .hgignore: >>>> -what's OpenJDK policy for these? I see we have .idea/* in there; how >>>> about we put .externalToolBuilders/* and .settings/* there instead? >>>> >>>> Block.java: >>>> - I don't like variables named "tmp" anything. "terminalFlags"? >>>> - New public constructors are entirely undocumented. Not that the old one >>>> had quality documentation, but still… >>>> - private constructor no longer sets finish. Why is it still in the >>>> parameter list? >>>> >>>> ForNode.java: >>>> - undocumented public constructor >>>> >>>> LoopNode.java: >>>> - undocumented protected constructor parameters >>>> >>>> WhileNode.java >>>> - undocumented public constructor parameters >>>> >>>> In all the new classes for parser context implementation: >>>> - add empty line between package declaration and imports >>>> - the import ordering doesn't follow our source code policy of "strict >>>> alphabetical, no grouping separator lines" >>>> - do these classes need to be public? >>>> >>>> Parser.java: >>>> - many local variables and method parameters could be declared final. >>>> - I generally dislike assigning an expression only used once to a local >>>> variable, e.g. >>>> List<Statement> statements = newBlock.getStatements(); >>>> return new Block(blockToken, finish, newBlock.getFlags(), statements); >>>> or >>>> FunctionNode scriptFunction = createFunctionNode(script, >>>> functionToken, ident, new ArrayList<IdentNode>(), >>>> FunctionNode.Kind.SCRIPT, functionLine, programBody); >>>> return scriptFunction; >>>> and there are more. >>>> >>>> BaseParserContextNode.java: >>>> - several cases of missing space before opening curly brace >>>> - missing "final"s on function parameters >>>> - missing documentation >>>> >>>> ParserContext.java >>>> - getLastStatement(): consider extracting common subexpression stack[sp - >>>> 1].getStatements() into a local variable >>>> - if you made push() generic as public <T extends ParserContextNode> T >>>> push(final T node) then you wouldn't need casting its return values in >>>> Parser. >>>> >>>> ParserContextFunctionNode.java >>>> - consider extracting the (flags & FunctionNode.SOME_FLAG) != 0 pattern >>>> into a boolean getFlag(final int flag) function >>>> >>>> Most of these remarks are related to stylistic issues: we try to ensure >>>> everything has a JavaDoc, we're strict about using the "final" keyword >>>> everywhere possible (you can have an Eclipse save action that adds them), >>>> we have a set policy for organizing imports so that different people's IDE >>>> settings don't cause unnecessary noise in diffs, etc. You'll get the hang >>>> of it as you go. Nice work overall! >>>> >>>> Attila. >>>> >>>> On Sep 15, 2014, at 5:01 PM, Andreas Gabrielsson >>>> <[email protected]> wrote: >>>> >>>>> Webrev: http://cr.openjdk.java.net/~lagergren/8012518.2/ >>>>> >>>>> https://bugs.openjdk.java.net/browse/JDK-8012518 >>> >> >
