<div style="font-family: Arial, sans-serif; font-size: 14px;"><span>Sorry for my slow working pace, I can only spare about a day a week for this, hope this is ok.</span><div><br></div><div><span>I'm not done yet, but I could use a little feedback.</span></div><div><br></div><div><span>I tried different versions of separating the addBuffer and flush procedures, playing with the sequence of things, etc. Long story short: you can break alot of other things and make it work as a Bugfix. But the reliable Bugfix for JDK-8229902 is not calling flush() in freeSpace(int size).</span></div><div><br></div><div><span>However, there is definitifely a problem in the buffer management. All I can say with confidence for now is that there are unnecessary flush-calls, and I strongly suspect also unnecessary addBuffer-calls. </span></div><div><br></div><div><span>So there is rendering time and possibly memory usage to be optimised. And maybe not just for Canvas, this touches a lot more in webkit rendering.</span></div><div><span>I'm inclined to see this as two different problems: 1) Bug JDK-8229902 and 2) RenderQueue Buffer management</span></div><div><span>Maybe improving Buffer management will at some point also solve 1), so I will first look into 2) and then see if 1) is still needed.</span></div><div><br></div><div><span>I havent looked much into the memory usage yet. What seems to improve rendering time the most is removing the flush()-call from WCRenderQueue::addBuffer(). And as far as I see, this doesnt even break anything. (there are some flushBuffer()-calls to be further analysed, some can be changed to flush(), some can be omitted. I still think, something in flush() needs fixing, but im not there yet. Still learing how to navigate and understand the jfx codebase)</span></div><div><span>This improves Canvas rendering time by about one third. And as far as I see, doesnt break anything. Now when I broke my build-setup last week, I tested it out on jfx-21. There I could see also improvements in rendering other things than canvas.</span></div><div><span>Back on the current Master of openjfx, that effect seems to go away, or at least hide in the margin of error. Overall, Webkit rendering time has significantly improved since 21. However, Canvas rendering time improvement with the change in WCRenderQueue on the current master is still at around one third.</span></div><div><br></div><div><br></div><div><span>So I'm hoping that if I understand the rendering time improvements done since jfx 21, I might better understand the problem at hand.</span></div><div><br></div><div><span>I could do something like a binary search for the change. But with my build-time, this will take me about a day. So another week.</span></div><div><br></div><div><span>If someone knows from the top of their head, what commit(s) significantly improved WebKit rendering time since jfx21, you could really save me some time. Maybe this is a change in Webkit, but I suspect its a change in how jfx uses webkit.</span></div><div><br></div><div><br></div><div><span>I mean, even if its just the Canvas rendering time improvement, that is something worth looking into, right?</span></div><div><br></div><div><br></div><div><span>thanks for any help or hints</span></div><div><br></div><div><br></div><div><span>kind regards</span></div><div><br></div><span>Dani</span><br></div><div style="font-family: Arial, sans-serif; font-size: 14px;"><br></div>
<div class="protonmail_signature_block" style="font-family: Arial, sans-serif; font-size: 14px;">
<div class="protonmail_signature_block-user protonmail_signature_block-empty">
</div>
<div class="protonmail_signature_block-proton">
Sent with <a target="_blank" href="https://proton.me/mail/home">Proton Mail</a> secure email.
</div>
</div>
<div style="font-family: Arial, sans-serif; font-size: 14px;"><br></div><div class="protonmail_quote">
On Friday, October 25th, 2024 at 1:28 PM, dani-kurmann <dani-kurmann@protonmail.com> wrote:<br>
<blockquote class="protonmail_quote" type="cite">
<div style="font-family: Arial, sans-serif; font-size: 14px;">Ok, the flush() function and its business in WCRenderQueue definitively deserve a closer look:</div><div style="font-family: Arial, sans-serif; font-size: 14px;"><br></div><div style="font-family: Arial, sans-serif; font-size: 14px;">I tried to do a proof-of-concept of how to test for this bug:</div><div style="font-family: Arial, sans-serif; font-size: 14px;"><span><a href="https://github.com/CodeMonkeyIsland/Bugtest-JDK-8229902" rel="noreferrer nofollow noopener" target="_blank">https://github.com/CodeMonkeyIsland/Bugtest-JDK-8229902</a></span><br></div><div style="font-family: Arial, sans-serif; font-size: 14px;"><br></div><div style="font-family: Arial, sans-serif; font-size: 14px;">Its in a "its not pretty, but it works... sometimes"-state. Not ready for anything. But I discovered something interesting:</div><div style="font-family: Arial, sans-serif; font-size: 14px;"><br></div><div style="font-family: Arial, sans-serif; font-size: 14px;">Bug.java is the adjusted test (not unit test) from <span><a href="https://bugs.openjdk.org/browse/JDK-8229902" rel="noreferrer nofollow noopener" target="_blank">https://bugs.openjdk.org/browse/JDK-8229902</a>, I passed the function in the html-document, because executeScript wasnt working. You can see the Bug there.</span></div><div style="font-family: Arial, sans-serif; font-size: 14px;"><span>In CanvasRenderingTest.java, I was testing out the testing strategy. Not very gracefully, I just handed over the pixelarray to be tested as a String and then decoded it into an int array again.</span></div><div style="font-family: Arial, sans-serif; font-size: 14px;"><span>I had some Problem with gc, I think, so I did the whole painting and extracting the imagadata with engine.executeScript(). - And the Bug doesnt show up anymore! ...</span></div><div style="font-family: Arial, sans-serif; font-size: 14px;"><span><br></span></div><div style="font-family: Arial, sans-serif; font-size: 14px;"><span>I tried with bigger canvas, but it seems to work, just my test breaks at some point ... gc again I'm guessing.</span></div><div style="font-family: Arial, sans-serif; font-size: 14px;"><span><br></span></div><div style="font-family: Arial, sans-serif; font-size: 14px;"><span>So definitively, the code is working as intended under some circumstances. So understanding when it works and when it doesnt work, probably means understanding the actual problem.</span></div><div style="font-family: Arial, sans-serif; font-size: 14px;"><span><br></span></div><div style="font-family: Arial, sans-serif; font-size: 14px;">Ill definitively have a deeper look into this sometime next week. And hopefully define the problem better.</div><div style="font-family: Arial, sans-serif; font-size: 14px;"><br></div><div style="font-family: Arial, sans-serif; font-size: 14px;">kind regards</div><div style="font-family: Arial, sans-serif; font-size: 14px;"><br></div><div style="font-family: Arial, sans-serif; font-size: 14px;">Dani</div><div style="font-family: Arial, sans-serif; font-size: 14px;"><span><br></span></div><div style="font-family: Arial, sans-serif; font-size: 14px;"><span><br></span></div><div style="font-family: Arial, sans-serif; font-size: 14px;"><br></div><div style="font-family: Arial, sans-serif; font-size: 14px;"><br></div><div style="font-family: Arial, sans-serif; font-size: 14px;"><br></div>
<div style="font-family: Arial, sans-serif; font-size: 14px;" class="protonmail_signature_block">
<div class="protonmail_signature_block-user protonmail_signature_block-empty">
</div>
<div class="protonmail_signature_block-proton">
Sent with <a href="https://proton.me/mail/home" target="_blank" rel="noreferrer nofollow noopener">Proton Mail</a> secure email.
</div>
</div>
<div style="font-family: Arial, sans-serif; font-size: 14px;"><br></div><div class="protonmail_quote">
On Monday, October 21st, 2024 at 6:19 PM, Kevin Rushforth <kevin.rushforth@oracle.com> wrote:<br>
<blockquote type="cite" class="protonmail_quote">
Hi Dani,<br>
<br>
Welcome, and thank you for taking a look at this problem.<br>
<br>
I don't know enough yet to suggest the best course of action, but I
took a quick look at it. I only see the one call to <span>the
native RenderingQueue::</span>flush -- from freeSpace(int) if
autoFlush is true. <span>RenderingQueue::</span>flush does a Java
upcall to WCRenderQueue::fwkFlush which then
calls WCRenderQueue::flush to decode and process the buffer. The
native <span>RenderingQueue::</span>flushBuffer method does a Java
upcall to WCRenderQueue::fwkAddBuffer which will add the buffer to a
linked list of buffers, and then allocate a new BufferData wrapper
object. It then calls WCRenderQueue::flush if the total size of the
linked list of buffers it too large.<br>
<br>
One thing that should be checked is whether the call to
WCRenderQueue::flush from native <span>RenderingQueue::</span>flushBuffers
and then again from <span>RenderingQueue::</span>flush is causing
the problem. Instrumenting the code on the Java side (which is
usually easier if it gives you the information you need), might show
what's going on.<br>
<br>
-- Kevin<br>
<br>
<br>
<div class="moz-cite-prefix">On 10/20/2024 7:01 AM, dani-kurmann
wrote:<br>
</div>
<blockquote type="cite">
<div style="font-family: Arial, sans-serif; font-size: 14px;"><span>Hi
everyone, nice to be here.</span>
<div><br>
</div>
<div><span>I don't want to be overzealous, but I did sign up for
a bugfix :)</span></div>
<div><span>So here is me, socializing:</span></div>
<div><br>
</div>
<div><span>I ran into the bug in a similiar scenario as
described in the Bug report: <a target="_blank" rel="noreferrer nofollow noopener" href="https://bugs.openjdk.org/browse/JDK-8229902" class="moz-txt-link-freetext">https://bugs.openjdk.org/browse/JDK-8229902</a></span></div>
<div><br>
</div>
<div><span>It's a buffer-problem. The culprit is the
flush()-call on line 64 of
/modules/javafx.web/src/main/native/Source/WebCore/platform/graphics/java/RenderingQueue.cpp
(in the freeSpace(int size) function)</span></div>
<div><br>
</div>
<div><span> if (m_buffer &&
!m_buffer->hasFreeSpace(size)) {</span></div>
<div><span> flushBuffer();</span></div>
<div><span> </span></div>
<div><span> if (m_autoFlush) { </span></div>
<div><span> flush();</span></div>
<div><span> }</span></div>
<div><span> </span></div>
<div><span> }</span></div>
<div><br>
</div>
<div><span>this gets called, before a shape(or possibly
something else) is added to the bytebuffer. If there is not
enough space left in the buffer, here we go:</span></div>
<div><br>
</div>
<div><span>The flushBuffer()-Call works fine. It sets up new
Buffer Space as intended and, I guess, flushes the buffer.</span></div>
<div><span>The flush()-call however leads to the described Bug.
Just comment it out to test it. But thats not a bugfix, of
course.</span></div>
<div><br>
</div>
<div><span>Its very interesting where the two calls lead, this
is right in the sweet spot between java and c++ and openjdk
and webkit. I dont claim to understand half of it. But I do
find it very interesting. </span></div>
<div><br>
</div>
<div><span>This is the point where I ask for help: I can propose
a bugfix, but I want to understand the issue deeper. Maybe
this is a newbie-problem, but I have problems understanding
the above mentioned sweet spot. So any advice is very
welcome!</span></div>
<div><br>
</div>
<div><span>Please DO correct me, and show me that other approach
that I dont see, but the way I see it, there's two ways to
approach this thing:</span></div>
<div><span>A) Treat it as an initialisation-problem. e.g. There
is nothing wrong with RenderingQueue, it just needs to be
initialised with m_autoFlush=false. (in this case, at least)</span></div>
<div><span>B) There seems to be something strange going on in
RenderingQueue, understand it and fix it.</span></div>
<div><br>
</div>
<div><br>
</div>
<div><span>A) is a lot easier: just follow back to where the
RenderingQueue Constructor gets called:
/modules/javafx.web/src/main/native/Source/WebCore/platform/graphics/java/ImageBufferJavaBackend.cpp,
line 82:</span></div>
<div><br>
</div>
<div><span>auto context =
makeUnique<GraphicsContextJava>(new
PlatformContextJava(wcRenderQueue, true));</span></div>
<div><br>
</div>
<div><span>The second Argument for the
PlatformContext-constructor sets the m_autoFlush-Variable
upon which the flush()-call in the freeSpace-function in
RenderingQueue.cpp is conditional. Set it to false.</span></div>
<div><span>as a git commit: <a target="_blank" rel="noreferrer nofollow noopener" href="https://github.com/CodeMonkeyIsland/jfx/commit/96f23307c7b6f324bac416b90c0eac4ad40b13a8" class="moz-txt-link-freetext">https://github.com/CodeMonkeyIsland/jfx/commit/96f23307c7b6f324bac416b90c0eac4ad40b13a8</a></span></div>
<div><br>
</div>
<div><span>pro: </span></div>
<div><span>-simple fix </span></div>
<div><span> -no need to mess around in more than one file</span></div>
<div><span> -looks like this is a normal way to use
RenderingQueue</span></div>
<div><span>con: </span></div>
<div><span> -more and more feels like a hack</span></div>
<div><br>
</div>
<div><br>
</div>
<div><span>B) is can of worms: You can comment out not only the
flush()-call in the freespace-function, but the flush()
function itself. At first, I thought, that the
flush()-function is still needed for a flush-call during
flushBuffer() outside RenderingQueue. But that was not the
same flush()-function. I just tested that, and it works.
(havent checked yet, if this breaks something else)</span></div>
<div><span>So assuming, the flush()-function is really defunct
and not needed(and I would have to dive into this a lot
deeper to make that statement as a fact), this is a chance
to do some spring-cleaning in RenderingQueue. Again, I only
see two main ways of approaching this:</span></div>
<div><span>- make m_autoFlush obsolete. Since (if the assumption
above is right) it already flushes regardless of how
m_autoFlush is set, lets stop pretending. Remove
m_autoFlush, adjust the constructors and the constructor
calls outside RenderingQueue.</span></div>
<div><span>- make m_autoFlush great again :) It would basically
mean separating flushBuffer() into two functions, one that
does the addBuffer-part and one that does the flushing. Then
do the addBuffer unconditional and the flushing conditional
on m_autoFlush().</span></div>
<div><span>The "problem" here being, that other parts of the
codebase seem to have gone the way of approach A). So
another piece of code initializing RenderingQueue with
m_autoFlush=false doesnt mean that they dont want to
autoFlush. So every call of the constructor has to be
analysed, if the intention is to autoflush or not. I dont
see the use-case for not flushing. So this approach might
very well lead to a situation, where every single
constructor call sets m_autoFlush to true and m_autoFlush is
kind of obsolete again. Or not. Anyone has an example, where
it shouldnt flush? Maybe there is runtime to be optimised by
planning when to flush? I dont know...</span></div>
<div><br>
</div>
<div><span>pro: </span></div>
<div><span>-if there is a problem in RenderingQueue, then thats
the place to fix it! No matter the consequences.</span></div>
<div><span>con: </span></div>
<div><span>-even if its an easy fix in RenderingQueue, it will
change its behaviour. Or at the very least make autoFlush
obsolete. This has consequences.</span></div>
<div><br>
</div>
<div><br>
</div>
<div><span>Approach B) seems a lot scarier than A), but if it
must be done, it should be done. If you guys think this is
the rabbit hole to be, I would be willing to go there.
However, I don't feel competent to overview all the
consequences of this. I could really need some advice and
help on that road, I guess thats why Im here.</span></div>
<div><span>I could imagine doing A) as a temporary fix and an
exercise. Then exploring B) deeper with some help.</span></div>
<div><br>
</div>
<div><span>As for testing: At first glance, this seems like a
non-automated Mk.1 eyeball-test. On a closer look however,
you could do something like the Bug-Class in the bug-report,
extract the RenderingContext-imagedata, pass it from js to
java and check the color of the pixel, to see if a shape has
been drawn. A window would have to be opened for this test,
I dont know if thats allowed. (But why not, I guess)</span></div>
<div><br>
</div>
<div><br>
</div>
<div><span>So what do you guys think? Does that sound like a
plan? Does this break something, that I don't see?</span></div>
<div><span>I am new here. Anyone willing to have a look into
this and maybe point me in the right direction sometimes?</span></div>
<div><br>
</div>
<div><br>
</div>
<div><span>kind regards</span></div>
<div><br>
</div>
<div><span>Dani Kurmann</span></div>
<div><br>
</div>
<div><br>
</div>
<div><br>
</div>
<div><span>p.s. the Bug class in the bug report does not work
anymore.(at least for me, forked or unforked) the script in
executeScript doesn't get executed. As a quick fix, you can
pass the function in the html document like so:</span></div>
<div><br>
</div>
<div><span> WebEngine engine = webView.getEngine();</span></div>
<div><span> String content="<html><body><canvas
id=\"canvas\" width=\"600\" height=\"600\" style=\"border:
1px solid
red;\"></canvas><script>window.onload =
function() {";</span></div>
<div><span> content=content</span></div>
<div><span> + " var canvas = document.getElementById('canvas');"</span></div>
<div><span> + " var c = canvas.getContext('2d');"</span></div>
<div><span> + " var size = 4;"</span></div>
<div><span> + " var step = size + 2;"</span></div>
<div><span> + " for (var y = 0; y <=
canvas.height; y = y + step) {"</span></div>
<div><span> + " for (var x = 0; x <=
canvas.width; x = x + step) {"</span></div>
<div><span> + " c.fillRect(x, y, size, size);"</span></div>
<div><span> + " }"</span></div>
<div><span> + " }";</span></div>
<div><span>
content=content+"}</script></body></html>";</span></div>
<span> engine.loadContent(content);</span><br>
</div>
<div style="font-family: Arial, sans-serif; font-size: 14px;"><br>
</div>
<div class="protonmail_signature_block" style="font-family: Arial, sans-serif; font-size: 14px;">
<div class="protonmail_signature_block-user protonmail_signature_block-empty">
</div>
<div class="protonmail_signature_block-proton"> Sent with <a rel="noreferrer nofollow noopener" target="_blank" href="https://proton.me/mail/home">Proton Mail</a> secure email. </div>
</div>
</blockquote>
<br>
</blockquote><br>
</div>
</blockquote><br>
</div>