Hi Laurent,
A couple of comments that might just be points for clarification. Minimally it would make sense to include the JBS
report number on the third item below, but the rest are just notes and suggestions...
On 4/26/17 2:32 PM, Laurent Bourgès wrote:
- MarlinProperties - TileSize vs TileWidth. Is there a reason you haven't
created a TileHeight property? I could
see a couple of ways of going here:
- TileSize means height and TileWidth is new which is just odd naming
In this case, I'd make the default for TileWidth be the value for
TileSize
otherwise setting tilesize used to set both W&H, but now it only sets
H?
- TileSize is legacy and new values are TileWidth and TileHeight
Both default to TileSize if not specified
In either case, I would think that TileWidth should default to TileSize?
Fixed, I adopted the first solution and getTileWidth_Log2() uses
getTileSize_Log2() to get the default value (W=H)
I was leaning towards adding W & H and having Size be the old mechanism - for
symmetry - but this is fine.
- MarlinTileGenerator,MarlinRenderer - all of the methods called on rdrF
and rdrD have the same signature. Why not
have MarlinRenderer include those methods and then you just need to store a
single MarlinRenderer field and be able
to manipulate either type...?
I agree. I tried but benchamrks proved that interface calls method were
slower than monomorphic calls so I adopted
this bimorphic call optimization where only 1 type is really used at runtime.
I'm curious how much difference this made to require this amount of complexity, but there is a solution here if you are
really worried about performance - use a super-class instead of an interface. If you can measure overhead for invoking
an abstract method then there is something wrong with the VM.
- Renderer, line 85,114 - maybe add a line saying that is the result of
<name> test? Did we put that test into the
repo anywhere?
Added comment: 'TestNonAARasterization: cubics/quads' (not in repo, only in
JBS)
I'd add the JBS number "JDK_NNNNNNNN" as well then.
...jim
