Jim & Phil Here is a new webrev: http://cr.openjdk.java.net/~lbourges/marlin/marlin-s3.4/
Changes: - Renderer: remove unused D_ERR_STEP_MAX and 0x1 replaced by 1 for readability - MarlinRenderingEngine: optimized loop in pathTo() - Dasher: TODO remains related to dash length issue Hope it is good now, I plan to work soon on the Array Cache & dispose code. Here are my comments to your emails: 1st email: 2015-08-13 22:32 GMT+02:00 Jim Graham <james.gra...@oracle.com>: > > On 8/10/2015 2:15 PM, Laurent Bourgès wrote: > >> Jim, >> >> Here is the new webrev including your proposals to use 32.31 fixed-point >> maths and double-precision in addLine() as it is definitely better: >> http://cr.openjdk.java.net/~lbourges/marlin/marlin-s3.3/ >> > > Why are ceil_int and floor_int implemented differently? > Fixed and tested OK. > > For CHECK_NAN, how fast is Math.isNaN? Is it worth first comparing the > intpart to the value that NaN converts to before calling isNaN? The > majority case, by a wide margin, is that the value is not NaN. > That's the aim: I test first the most probable case (a <= intpart) for ceil() and then check possible overflow and finally check for NaN values to avoid adding / substracting 1 ! I made benchmarks and this implementation optimized for the integer domain works very well: 2x faster. > > Helpers.java, line 214 - why move the declaration into the for loop? That > is confusing to me since "j" is not used in the for() conditions. > Reverted, sorry. > MarlinConst - It looks like the subpixel sizes are now properties. Did > that have performance consequences? The values are no longer true literal > constants when compiling the code now. > In Marlin, several constant values are customizable by system properties for some time ! However, hotspot considers them as "proper" constants and performs its optimizations at runtime to final static fields => inlined as fast as true literals. 2nd email: > 2015-08-13 22:35 GMT+02:00 Jim Graham <james.gra...@oracle.com>: > > On 8/10/2015 2:15 PM, Laurent Bourgès wrote: > >> Renderer.java, line 792: Do you need to copy ERR_STEP_MAX into a >> local? It is a statically initialized final int - that should >> already be a constant that gets copied into the code. >> >> >> I always copy constants used in loops to avoid repeated constant lookups. >> > > I was under the understanding that if a constant is declared final and > static, then it is compiled directly into the code and there is no constant > lookup. Check the bytecodes and you should see a hard-coded literal > 0x7fffffff everywhere it is used... > I agree but it seems slightly faster: probably loading the literal is a bit slower than loading from the stack ? Maybe some hotspot experts could give their opinion. As this is the renderer hot loop, I prefer making it as fast as possible. 3rd email: 2015-08-13 22:45 GMT+02:00 Jim Graham <james.gra...@oracle.com>: > Is multiplying by POWER_2_TO_32 really faster than scalb? Quick power of > 2 multiplies was supposed to be what scalb was good at, but if they didn't > make it a Hotspot intrinsic then it may not work as well... > I do not know if there is an intrinsic implementation but it is 2x faster in my microbenchmarks. I looked at Math.scalb() implementation and it has several conditional checks (huge exponents handled in while loop) and it computes 2^n at every call: that's why I precompute the correct scaling factor once ! > > Renderer.java, line 807 - missing space before = > Fixed. 4th email: 2015-08-13 22:56 GMT+02:00 Jim Graham <james.gra...@oracle.com>: > What is with the new added "default" cases? Do they help the compiler? > It just fixes several Findbugs warnings without any side-effect. > I'll defer to Phil on the naming of the new system properties > (sun.java2d.renderer.*). Eventually Marlin may replace both Pisces and > Ductus and then the generic names make sense. Should they have "marlin" in > the name until then? > This is not new and present in the first Marlin patch. However, these properties are helpful to tune Marlin settings (performance and quality). I could remove some properties in the future once Marlin will be tested enough and its tuning done. Phil's email: 2015-08-13 23:20 GMT+02:00 Phil Race <philip.r...@oracle.com>: > On 08/13/2015 01:56 PM, Jim Graham wrote: > >> What is with the new added "default" cases? Do they help the compiler? >> >> I'll defer to Phil on the naming of the new system properties >> (sun.java2d.renderer.*). Eventually Marlin may replace both Pisces and >> Ductus and then the generic names make sense. Should they have "marlin" in >> the name until then? >> > > I think that at least for now they should either be > sun.java2d.renderer.marlin.* or > just sun.java2d.marlin.* > Ok, I can rename them in the next webrev as it requires me to upgrade all my testing & benchmarking scripts... > But the more important question may be around how much benefit these > bring compared to the additional testing overhead of so many combinations ? > I advocate it is not easy to test all combinations but it seems not a reasonable testing objective. As I said, it is mainly tuning or quality settings but only few are important (subpixels X / Y, tile size, initial pixel size). Jim, thanks for all your efforts & take care, Best regards, Laurent
