Initial run through. Ill take a deeper look after I finish up some year
end todo's.


http://gwt-code-reviews.appspot.com/127801/diff/1101/1106
File
reference/Microbenchmarks/src/com/google/gwt/reference/microbenchmark/client/Microbenchmarks.java
(right):

http://gwt-code-reviews.appspot.com/127801/diff/1101/1106#newcode35
Line 35: public class Microbenchmarks implements EntryPoint {
Consistent CamelCasing -> MicroBenchmarks.

http://gwt-code-reviews.appspot.com/127801/diff/1101/1108
File
reference/Microbenchmarks/src/com/google/gwt/reference/microbenchmark/client/TestDom.java
(right):

http://gwt-code-reviews.appspot.com/127801/diff/1101/1108#newcode25
Line 25: static final String HTML = "<div>"
jgw found that white space (new lines count) can introduce noise into
the tests (at significant performance penalty) since they stick text
nodes into the tree.

To make sure you are comparing apples to apples DOM structure wise,
Please use a predictable string with known amounts of whitespace
(preferrably non since it is hard to count whitespace). I would
recommend just having a string on a single line with some random text
inside some nodes, and then letting eclipse autoformat handle the string
wrapping.

http://gwt-code-reviews.appspot.com/127801/diff/1101/1110
File
reference/Microbenchmarks/src/com/google/gwt/reference/microbenchmark/client/TestDomBinder.ui.xml
(right):

http://gwt-code-reviews.appspot.com/127801/diff/1101/1110#newcode1
Line 1: <ui:UiBinder xmlns:ui='urn:ui:com.google.gwt.uibinder'>
Again, comment about whitespace. Not sure if UiBinder trims it or not.

http://gwt-code-reviews.appspot.com/127801/diff/1101/1111
File
reference/Microbenchmarks/src/com/google/gwt/reference/microbenchmark/client/TestFlows.java
(right):

http://gwt-code-reviews.appspot.com/127801/diff/1101/1111#newcode7
Line 7: public class TestFlows extends Composite {
Is flow panel nothing more than a thin wrapper around a DIV?

Should probably document this so we know what we are comparing.

http://gwt-code-reviews.appspot.com/127801/diff/1101/1115
File
reference/Microbenchmarks/src/com/google/gwt/reference/microbenchmark/client/TestWidgetBinder.ui.xml
(right):

http://gwt-code-reviews.appspot.com/127801/diff/1101/1115#newcode2
Line 2: xmlns:gwt='urn:import:com.google.gwt.user.client.ui'>
Double check this to make sure the resulting DOM + whitespace is
comparable to the other tests.

http://gwt-code-reviews.appspot.com/127801/diff/1101/1116
File
reference/Microbenchmarks/src/com/google/gwt/reference/microbenchmark/client/WidgetCreation.java
(right):

http://gwt-code-reviews.appspot.com/127801/diff/1101/1116#newcode63
Line 63: }, new Maker("FlowPanel") {
Should probably also include these textual descriptions in the
individual test case classes. Makes looking through them easier.

http://gwt-code-reviews.appspot.com/127801/diff/1101/1116#newcode75
Line 75: }, new Maker("Complex UI, pure dom, get children by id") {
Pure DOM? This test seems to use innerHTML to create the DOM structure
on an unattached div. Why not call it InnerHTML on unnattached div,
getChildrenById.

Maybe writting a name and a longer description and exposing them as
String constants in the test files would be nice.

http://gwt-code-reviews.appspot.com/127801/diff/1101/1116#newcode178
Line 178: long start = System.currentTimeMillis();
For a time sensitive benchmark framework, I snub my nose at looping in
the long emulation. Why not just have a simple private utility method:

   private static native double currentTimeMillis() /*-{
     return (new Date()).getTime();
   }-*/;

http://gwt-code-reviews.appspot.com/127801/diff/1101/1116#newcode181
Line 181: r.add(widgets[i]);
There are several things that you may want to measure here.

1. Synchronous Time to add X instances.

2. Time with the end time taken via setTimeout(grabEndTime,0) as an
attempt to include the deferred work that can happen after the JS runs,
but before the user has a chance to interact. This can be drastically
affected by removing the widget since the subsequent deferred style
matching doesnt have any work to do.

3. The time to add and remove a widget synchronously.


I would argue that 1 and 2 are more important than 3. It would be nice
if each test run included all 3 measurements.

http://gwt-code-reviews.appspot.com/127801

-- 
http://groups.google.com/group/Google-Web-Toolkit-Contributors

Reply via email to