Performance and correctness fixes across core, the Apple ports and the VM - #5686
Merged
Conversation
…e VM Start-up and per-frame cost, plus three crashes and a stale-artifact bug. Every measurement quoted here is from a native Mac build, and the reasoning behind each change is in the code rather than here. Start-up - convertToPixels asked the main thread which monitor the window is on, through a synchronous dispatch, for every padding and every margin on every component. The screen and its scale are now published as one value when the window is built and whenever it moves, so the query reads an atomic instead. 35ms of blocked event dispatch thread per launch. - AppKitWindowManager's constructor marshalled an observer INSTALL synchronously while the main queue was still bringing AppKit up. Nothing reads it back, so it no longer waits. 37ms. - monitorEnter cleared threadActive -- announcing a GC park -- before it knew whether the lock would block, so even an uncontended lock waited out the collector's handshake. It now tries the mutex first and only announces a park it actually performs. 8.7ms. The handshake loop is also instrumented, because the stall report could not see it and was reporting zero. - UIManager scanned the whole theme table once per distinct UIID to answer whether a dark variant exists. The dark keys are indexed once per theme generation: first use of a UIID went from 111,955ns to 17,378ns. - Switch built its artwork -- including a gaussian blur -- to answer getPreferredSize, so a switch that is never painted still paid for it during layout. The size is derived from the same numbers directly. Rendering and memory - The Metal image pipeline no longer round-trips every picture through the CPU, and no longer keeps a decoded copy of each EncodedImage beside its GPU texture. Rounded corners are done in the shader instead of building a rounded copy of the image. Crashes and correctness - A null socket handle unboxed to a long and took the process down. - createImageFromARGBImpl was half-merged; its one-pass premultiply is restored. - The mangled name of the rounded-draw capability native was wrong, which the linker cannot catch: the symbol simply went missing and the feature was inert. - Forked Maven invocations (cn1:run, cn1:debug) did not inherit -Dmaven.repo.local, so a child silently resolved from the default repository and ran stale artifacts. Verified: core 6145/6145 tests; android, ios, mac, javase, the maven plugin and ByteCodeTranslator all build; SpotBugs zero findings in every gated module (core-unittests regenerated, not read stale); cast-semantics, native-signature and build-hint gates clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61c6c753fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 12 screenshots: 12 matched. |
Collaborator
Author
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
derive() is cheap, but the native font it stands for is built on first use -- inside create(), on the stringWidth call. A screen of icons therefore paid for a native font PER ICON even when every one of them wanted the same size: measured on a native build at 149us per icon, 7.3ms across 49 icons, the largest single cost in building the screen. A Font is immutable and Codename One already shares font instances, so one derived font can serve every icon of that size. Both callers derive through it. createMaterial(char, Style) went through Font.derive directly on every icon, so it paid the same cost as the sized overload and is fixed by the same change. The cache is deliberately not a plain static map: - It is held through createSoftWeakRef, exactly as Image holds its scale and rgb caches. These fonts are an optimisation and nothing else refers to them, so a device under memory pressure is entitled to reclaim them and pay the derive again rather than keep a native font per size alive for the life of the process. - It remembers the font it was derived FROM. A theme that registers a different icon font invalidates every size derived from the old one, and comparing the base is cheaper than tracking a generation. - It is bounded. The key is a pixel size, so an app that animates one -- a zooming or continuously resized icon -- would otherwise add an entry per frame that the soft reference cannot drop until memory actually runs short. Past the limit the derive still happens, it is simply not remembered. Tested for each of those: icons of one size share a font, a different base font invalidates, and a cleared soft reference rebuilds instead of returning null. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Cloudflare Preview
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a821b397d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
single-buffered present - CN1Metalcompat getRGB read the staging texture from the SOURCE origin. The blit copies the requested subregion into a readW x readH scratch at (0, 0), so a read starting at (readX, readY) runs off the end of it and Metal fails the readback for any ordinary non-origin subregion. The pixel loop below it already treated the scratch as zero-based. - Switch derived its preferred size from the font and the scale constants even when the THEME supplied the artwork. Those images are whatever size the theme made them, so a custom thumb was under-measured and clipped. Whether the theme supplied them is recorded when the theme is read, because the generated images are cached into the same fields and a null check cannot tell them apart later. Generated artwork still skips the rasterisation; themed artwork is measured, which is the only way its size can be known. - Graphics.drawImageRounded handed the raw native peer to the port. Drawing an Image is a virtual call: ComponentImage, DynamicImage, FontImage, RGBImage and SVGScaledView paint procedurally and several have no peer at all, so the rounded draw rendered nothing; EncodedImage needs its decode hook; and a rotated image keeps its angle beside the peer, so it drew unrotated. Images now answer with a peer only when that peer is the whole picture, and anything else falls back to the normal square draw the method already promises where a platform cannot round. - UIManager's float fast path accepted a field with more than seven fractional digits and then stopped accumulating, so 0.123456789 became the float for 0.1234567 -- a different margin. It now declines and lets the fallback parse it exactly. - The macOS renderer presented through ONE IOSurface. Handing the layer a surface does not wait for the window server to finish reading it, so the next frame could land on top of a composite still in progress. It now alternates between two, which is the least a single-writer/single-reader handoff needs and still one buffer fewer than the drawable pool this replaced. Also the two gates the branch was failing: three files it modified had no complete licence header, one .metal had only the notice, and three new methods used classic Javadoc where this tree requires /// markdown. Collections.java is Apache Harmony and keeps its own notice, so it joins the other Harmony files in the header check's exclusion list rather than being re-licensed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59ec540581
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
monitorEnter now tries the mutex before announcing a GC park, and the Windows compatibility layer had init/lock/unlock but no trylock, so every Windows target failed to compile: 'call to undeclared function pthread_mutex_trylock'. TryAcquireSRWLockExclusive is its exact counterpart -- pthread_mutex_t is an SRWLOCK here -- and EBUSY is what a failed trylock reports. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Compared 166 screenshots: 166 matched. |
Collaborator
Author
|
Compared 166 screenshots: 166 matched. |
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
Contributor
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
PMD (all four forbidden violations were introduced by this branch): - Style's single-listener fast path compared with == before falling back to equals(). EventDispatcher's own ArrayList uses equals for both contains and remove, so the identity test was redundant AND less faithful; it is gone from both addStyleListener and removeStyleListener. - UIManager's dark-key scan is a foreach. - Switch's ImageFactory test is deliberately an IDENTITY comparison -- the question is whether this context was handed a different factory INSTANCE, and two equal-but-distinct factories still produce images that must not be shared -- so it keeps == with the reason in the code and a NOPMD marker, matching the existing suppressions in maps/ and routing/. JavaScript binding: this branch exposes Class.getSuperclass() as a native, and a new vm/JavaAPI native needs BOTH a JavascriptNativeRegistry entry and a bindNative in parparvm_runtime.js. Without them the translator classifies the symbol as uncategorized and emits a stub that throws at runtime. The binding returns null for an interface, which is what the C implementation does: a class file records java/lang/Object as an interface's super_class, so the isInterface flag is the only thing that separates them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e449cd8de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
The safe-area snap wrote its padding without announcing it, to stop a revalidate treadmill: a padding change is what Component.styleChanged answers with revalidateLater() on the parent, so laying out a safe-area container queued another revalidate of the whole Form, which laid it out again. On a busy event dispatch thread that measured ~200ms per pass against ~15ms of real painting. The treadmill is real, but those passes are load-bearing. The JavaSE video peer fills its buffer from the AWT side and never asks for a repaint itself -- Peer.paint deliberately calls paintOnBuffer rather than cnt.repaint to avoid a loop -- so it depends on something else repainting the form. Suppressing the announcement took that away: measured on the CEF/FFmpeg smoke, the peer painted 9 times against 208 before, and a video that decoded 18 frames of solid red (videoAverageColor=253,0,0) put not one red pixel on screen. So the announcement is restored and the reason is recorded at the call site, including why the obvious optimisation is wrong until peers drive their own repaints. The test that pinned the suppression now pins the opposite, so reinstating it fails loudly rather than silently blanking peer content. Verified: the smoke's screenshot is back to 1184836 red pixels, exactly master's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9163660bfc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… gone The soft-reference budget this branch added charged each entry by measuring it, and trimSoftRefs measured AGAIN when evicting. Measuring an Image means asking it for its width, and a soft-referenced image is precisely the kind that may have been disposed since it went in -- its native peer is then null, and on this VM reading a field through a null reference is a SIGSEGV rather than a catchable NullPointerException, so there is no defensive form of the recompute. It crashed the macOS screenshot suite: EXC_BAD_ACCESS at 0x20 inside softRefSize, reached from Image.cacheImage while a transition test scaled images, taking down 118 tests and leaving 61 unrun. It needs sustained image churn to show, which is why only the animation suites hit it. The size is now recorded when the entry goes in -- while the caller still holds the object and its peer is certainly alive -- and eviction subtracts the recorded number without touching the value at all. softRefSize therefore has exactly one call site, on the insert path. Also drops dead code the bug was hiding behind: the key is a fresh Object, so the "previous entry" branch in createSoftWeakRef could never run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b919e398b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 160 screenshots: 160 matched. Benchmark Results
Detailed Performance Metrics
|
- The soft-reference budget evicted down to nothing, so a value larger than the whole budget (a 3840x2160 decoded image is ~32MB against 24MB) threw away the entry createSoftWeakRef had just inserted; its key never resolved and the caller re-decoded the same image every frame. It now keeps the most recent entry: the budget is a target for the SET, not a cap on any one member. - The shared switch artwork cache was unbounded. Its key carries dimension, colour, inset and enabled state, so restyling a switch -- or making many short-lived ones -- retained an obsolete mutable image, and a native texture with it, for the life of the process. Bounded at 64. - ...and it was keyed on the theme generation alone, so installing a new global ImageFactory left a matching key answering with artwork the OLD factory made. The default factory's identity is now part of what invalidates it. - parseStyle dropped the plain cached style but not the prefixed one, so re-parsing an id left pressed/disabled/custom prototypes answering from the previous parse and the replacement was silently ignored. - The macOS present handed the layer its surface from a completion handler without checking that the surface was still current. A resize between commit and completion rebuilt the surfaces, and the stale one was then displayed stretched until something newer landed. Frames now carry the generation they were drawn for and superseded ones are dropped. - getDevicePixelRatio derived the scale from getDeviceDensity. That is wrong in BOTH of that method's modes, not only under ios.densityOld: the density bucket approximates DPI and is chosen from the display RESOLUTION, so a 750x1334 2x phone lands in a bucket implying 3, and some iPads landed in a bucket this had no case for and answered 0. It now asks the platform through a new getDisplayScale() native that reads UIScreen.scale. - A Metal memory warning bumped the NATIVE texture generation only. That is recoverable for an image holding its decoded copy, but one created through createImageNoBackingCopy has released it deliberately, so it had no pixels and nothing telling it to rebuild -- it kept returning the discarded texture and drew corrupted. The memory warning now bumps the Java decode generation too, the same call applicationWillResignActive already makes; that pairing simply did not cover this path, and the comment claiming otherwise was wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6114c1839c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…with the Mac Two defects in the previous two commits. monitorEnter's uncontended try-lock returned without ever looking at threadBlockedByGC. Skipping the ANNOUNCEMENT is right -- a thread that does not block need not tell the collector it is parking -- but skipping the HANDSHAKE is not: monitorEnter is one of the few points a long-running loop is guaranteed to reach, so a loop whose only safepoints are non-reentrant synchronized blocks could run forever while a stop-the-world collector waited for it, stalling allocation behind it and hanging the application with no output at all. The fast path now honours a pending handshake exactly as the blocking path does; with no collection in flight it costs one relaxed read. This is the shape of the Linux suite wedge -- the app stopped emitting output entirely, for 38 minutes, after a transition test -- which I had wrongly written off as environment-correlated because the musl arch happened to pass. And getDisplayScale used [UIScreen mainScreen]. IOSNative.m is shared with the Mac port, where there is no UIScreen at all, so every macOS and watchOS build failed to compile. scaleValue is already the captured screen scale on every Apple target, so the fallback bought nothing; 0 means "not captured yet", which the Java caller already answers by deferring to the portable implementation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d948e9469
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Dropping a frame rather than overwriting a surface the compositor still holds was only half an answer. drawFrame ignores presentFramebuffer's result and marks the frame painted either way, so if the dropped frame was the last of a burst and the application then went idle, nothing asked to paint again and the window kept showing the previous frame until some unrelated change happened to repaint. Trading a torn frame for a permanently stale one is not an improvement. A dropped frame now records that one is owed, and the completion handler that frees a surface pays it by asking the framework to paint again. It wakes the framework rather than re-presenting from the callback. The dropped content is still in screenTexture, but that texture belongs to the thread that paints, and blitting it from the Metal completion thread would race the next frame drawing into it. Requesting a repaint puts the work back where it belongs and the existing path presents it. This is the same reasoning -- and the same pair of calls -- that updateBackingSize already uses after a resize: a layer-hosted view has no drawRect:, so setNeedsDisplay: alone reaches nothing and the framework has to be woken. The flag is cleared when the surfaces are rebuilt, because that path asks for a repaint of its own and would otherwise be asked twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d59a12400
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The soft-reference budget added here evicted from the table that backs createSoftWeakRef, and that table is not only an image cache. createSoftWeakRef is the portable way to hold something weakly, and callers use it to track LIFETIME as much as to cache results: JavascriptContext keeps every live JSObject wrapper there and treats a null extractHardRef as proof the wrapper was collected, calling release() on the underlying JavaScript object and dropping the entry. So once decoded images pushed the shared table past the budget, LRU eviction could remove a live JSObject's entry. The next cleanup() then read that as a death, released the JavaScript object underneath it, and left a wrapper that is still strongly reachable pointing at freed state. Eviction is now confined to values a caller can simply build again -- a decoded image and the pixel or byte arrays behind it -- which is what the budget was ever about. Everything else is inserted uncharged and is never evicted, so it lives exactly as long as it did before any budget existed. The loop counts charged entries rather than the whole table, or it would believe it still had somewhere to go while every remaining entry was one it must not touch. isRebuildableSoftRef is deliberately a whitelist. A kind of value nobody anticipated defaults to being kept, which costs memory; defaulting the other way would corrupt whoever was relying on it, which is this bug. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ite lost The single-pass premultiply replaced a path that ended in CGBitmapContextCreateImage with a direct CGImageCreate, and passed NO for shouldInterpolate. An image from a bitmap context carries YES, so every image built through Image.createImage(int[], w, h) silently started sampling nearest-neighbour instead of interpolating. Invisible at 1:1, which is how it got this far -- it only appears when such an image is drawn at a size other than its own, and then it affects the whole picture rather than its edges: a smooth gradient turns blocky. The watch UI suite caught it as graphics-draw-image-rect-image-aa-off/on, where the two variants that differ are exactly the two built from ARGB and drawn at half size, while the same images at 1:1 a few rows above are pixel-identical. Confirmed rather than reasoned about: CGImageGetShouldInterpolate reports YES for an image from CGBitmapContextCreateImage and NO for CGImageCreate with this argument, so the flag really did flip. The premultiply arithmetic itself was checked at the same time and is bit-exact against the CoreGraphics pipeline it replaced -- 0 of 65536 pixels differ over a full radial gradient with varying alpha -- so only the flag was wrong, not the conversion. The other CGImageCreate in this file sits inside the CN1_VERIFY_ARGB block and never renders anything, so its flag is irrelevant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d99c1dc4ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
registerBundledFont memoises which font files it has handed to Core Text in a function-local static NSMutableSet, created lazily and mutated with no synchronisation. Font.createTrueTypeFont is public, carries no thread restriction, and neither it nor loadTrueTypeFont holds callers to the event dispatch thread -- loading fonts on a background thread while the UI comes up is an ordinary thing for an application to do. Two threads reaching this at once would build the set twice or mutate it concurrently. Core Codename One deliberately has no locks: it runs on one event dispatch thread and its state does not need them. This is the other side of that rule rather than an exception to it -- the native boundary is where a call from an arbitrary thread actually arrives, and an NSMutableSet mutated from two of them is not a stale read the way a Java map would be. It is undefined behaviour that crashes. dispatch_once for the creation, and the lock spans the registration rather than only the membership test: releasing it straight after the add would let a second caller for the same file conclude it was already registered and ask Core Text for the name while the first was still parsing it. Nothing measurable is serialised -- a font file is registered once in the life of a process, which is the whole point of the set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63650b858b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
materialAtPixels reads a soft reference, may publish a fresh HashMap over it, and then does a get and a put on that map -- all without holding anything. Two threads deriving different icon sizes could publish a map over each other, resize one another's, or read one that was only partly built. This is not a lock added to core against the grain. Codename One runs on one event dispatch thread and its state needs no locks, but fonts are the exception the class already made: getMaterialDesignFont() is static synchronized on master and has been all along, because nothing holds an application to the event dispatch thread when it derives a font. materialAtPixels calls it as its first statement, so the method was ALREADY taking the class monitor -- only the part that reads and fills the cache had been left outside it. Extending the same lock over the whole lookup therefore costs nothing measurable. The call serialised on this monitor either way, and what it now guards is a map lookup whose miss is a font derive that dwarfs it -- which is the reason the cache exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6c3398b83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…surfaces The retry added for a dropped frame was reachable from exactly one place: a Metal completion handler freeing a slot. But a frame can be dropped with no GPU work outstanding at all -- every surface held by the window server, nothing in flight -- and the compositor releases a surface without a callback of any kind. In that state no completion was ever going to run, so the owed frame sat owed and the window kept showing the previous one until something unrelated repainted. The earlier fix removed that hole for one cause of exhaustion and left it open for the other. A drop now checks whether anything is actually in flight. If something is, its completion still pays the frame back as before. If nothing is, this schedules the retry itself, one display interval later -- immediately would only spin, since the condition that caused the drop is still true at that instant, and a sixtieth of a second is long enough for the compositor to have let go. Both paths go through cn1PayDeferredPresent, which clears the owed flag atomically before acting, so a completion and a timer that overlap ask for one frame rather than two. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecfde45ad4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The deferred-present retry paid every frame back with repaintUI(), which repaints Display.getCurrent() -- the main form. A secondary macOS window has its own METALView and its own surfaces, so when one of those deferred a presentation the retry dirtied a surface that had nothing to do with it: the flag was cleared, no frame was produced for the window that owed one, and it kept showing the old pixels until something else repainted it. The view already knows which window it is -- cn1WindowId, -1 for the host's view and >= 0 for a created one, the same test the pointer and key paths use. A secondary window is now asked for by id through a callback modelled on windowContentReadyCallback beside it, which looks the window up in Desktop and repaints it. Only native calls that method, which is also what keeps it: the translator retains a method whose mangled name appears in the native sources. Also fixes an ordering mistake made while writing this: the readiness check ran AFTER the flag was cleared, so a retry arriving before Java was up would drop the frame while recording it as paid -- the same failure the retry exists to prevent. Readiness is checked first now, and the flag stays set until there is somebody to ask. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b82b410de7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Neither existing test covers the window between assigning layer.contents and Core Animation handing that transaction to the window server. IOSurfaceIsInUse() answers for the server, which does not own the surface yet and so reads false, and the in-flight flag is already clear because the GPU finished before the handler ran. For that window a surface about to be displayed looked free to both tests, and a renderer quick enough to wrap the three-slot pool could draw over the frame being shown. Rather than try to observe the handoff, the slot most recently handed to the layer is simply never a legal target. It does not matter who owns it: the surface under the layer is not somewhere to draw, and it stops being current the moment a later frame replaces it. That covers the ownership gap and the handoff gap with one comparison and no new callback. Cleared to -1 wherever the surfaces are rebuilt, since the surface it named no longer exists, and set to 0 where the first one is published to the layer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8c6d876c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two problems with the same root. The build loop skips a slot only when IOSurfaceCreate fails, so a newTextureWithDescriptor that returns nil under GPU pressure leaves a surface behind with no texture -- and selection checked only the surface, so it would choose that slot and immediately give up on the frame. Worse, the giving-up there was hand-rolled: it committed and returned NO without recording that a frame was owed, so unlike the exhausted-pool case nothing ever asked for it again and drawFrame marked it painted regardless. Selection now requires a usable texture as well, so the dead slot is skipped while the good ones are still available. And every path that abandons a present goes through cn1DeferFrameAndCommit, which records the owed frame and arranges the retry. Two paths doing that separately is exactly how these drifted apart: the deferred flag was added to one of them and the other kept the older behaviour of losing the frame. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80a31d0374
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The published-scale accessor fell back to the primary display's scale until the main window published its own. The main window carries setFrameAutosaveName, so it reopens wherever it was last left -- which need not be the primary display -- and on a desktop whose monitors have different backing scales that fallback is a guess that can be wrong. Resources sized from it during start-up stay wrong after the window publishes the real value. That is the same failure as the placeholder 1 this accessor was written to remove: a number that looks authoritative and is not. Refusing outright would give up a correct answer in the ordinary case, since the primary display cannot be the wrong one when it is the only one. So uniformity is published alongside the scale -- whether every attached screen agrees -- and the fallback applies only then. Where they disagree this returns 0, "not captured", which the Java caller already answers by deferring to the portable implementation. The flag is published where the primary scale already was: once before the window is built, so it is available as early as the scale it guards, and again on NSApplicationDidChangeScreenParametersNotification so attaching a display with a different scale re-evaluates it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Signed-off-by: Shai Almog <67850168+shai-almog@users.noreply.github.com>
… paths
This was measurement scaffolding from the work that produced these fixes. It
should never have been left in, least of all on the paths it was measuring.
Removed from the per-frame path:
- CN1_REPAINT_RATIO in the GL view controller, which ran a cached getenv,
two nested branches, static counters and floating point on EVERY frame, and
printed to stderr every fifth one.
- CN1_GPU_TIME in the iOS Metal view, the same shape, and it attached a Metal
completion handler per frame while enabled.
- The firstDraw and firstPresent one-shot probes in the Mac drawFrame and
presentFramebuffer, which cost a branch on every frame for the life of the
process in order to log once.
Removed from the lock path:
- The stall instrumentation around the GC handshake in monitorEnter, which is
entered on every synchronized call. It expanded to nothing without a probe
define, but instrumenting the hottest path in the VM to answer a question
that has been answered is not a thing to leave behind. monitorEnter now
differs from master by comments only.
Also removed CN1_VERIFY_ARGB, a fifty-line CoreGraphics reference comparison
run per image creation, and ten cn1StartupPhase probes emitting BENCH: lines.
Those measured start-up for a comparison that is no part of this branch.
Master's own cn1StartupPhase("main") and its stall infrastructure are left
exactly as they were; only the call sites added here are gone.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…it path Debug instrumentation had no business in any of these and should not have been in the branch at all. Display.edtLoopImpl carried six `if (edtTrace)` blocks through the event dispatch loop, timing each phase into nine accumulator fields with a reporter that logged once a second, plus a setProperty hook to switch it on at runtime. Display now differs from master only by getDevicePixelRatio and isRoundedImageSupported, which are the actual API this branch adds. Form.flushRevalidateQueue timed every container it revalidated -- a ternary and a static call per container per pass, with a log for slow ones. Form is now identical to master; the hasPendingRevalidations helper added with it went too, since nothing ever called it. JavaSEPort timed the simulator's blit at three points and carried a 54-line reporter printing BLITTRACE lines once a second. Nothing measured here is worth a branch on the framework's hottest loop, and the questions this was built to answer have been answered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e448166bc1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
createSoftWeakRef on iOS is not a weak reference: the SoftReference line is commented out and the value goes into a plain Hashtable that holds it hard until something calls flushSoftRefMap. So decoded images accumulate for the life of the process while every caller believes it stored something reclaimable. The budget added here -- 24MB, LRU eviction, sizes charged at insert -- was aimed at that. It was the wrong mechanism: - It changed a contract callers were written against. On iOS extractHardRef had never returned null for a live key, so nothing was prepared for an entry to vanish. JavascriptContext keeps live JSObject wrappers in that same table and reads null as proof of collection: eviction made it release the JavaScript object under a wrapper that was still reachable. The type whitelist added afterwards was a guess at which entries in a shared namespace are safe to drop, not a fix for that. - 24MB is an invented number standing in for memory pressure the platform already reports. - It overloaded one table that serves both rebuildable caches and lifetime tracking. Reverted to master's implementation exactly. The growth is real and worth fixing, but in a cache of its own or driven by the platform's memory warning -- not by making a shared table forget things its callers assume it remembers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a63cc8e352
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review asked for a per-slot Core Animation reservation held for every committed transaction, on the grounds that a second frame's completion replaces this single value and frees the first slot while its transaction is still awaiting handoff. That is not reachable, so this records the reasoning rather than adding the bookkeeping. A CALayer holds one contents value, not a queue of pending ones. When a later completion assigns over an earlier surface, the earlier assignment is superseded before the window server ever sees it. So an older slot is only ever in one of two states, and both are already covered: the server latched it, in which case IOSurfaceIsInUse reports it and selection skips it -- or it never latched it, in which case that surface will not be displayed and reusing it is correct. What makes it safe is the pairing. contents and the reservation are assigned together in the same handler, and the in-flight flag is cleared only afterwards, so the slot the layer points at is covered by one or the other at every instant, including the gap between the two stores where the slot is still in flight. Completion handlers on one queue also run in submission order, and even out of order the invariant holds, because each handler sets both values itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b4d0d918b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+3110
to
+3113
| d.addListener(singleListener); | ||
| singleListener = null; | ||
| } | ||
| listeners = d; |
There was a problem hiding this comment.
When a worker thread changes a style while another thread adds or removes a listener, this unsynchronized promotion can permanently lose the concurrent update. For example, after the worker copies singleListener and clears it but before publishing listeners, an EDT registration can store a new singleListener; the worker then publishes its dispatcher, and all subsequent notifications ignore the newly registered listener. The previous implementation routed every registration through synchronized EventDispatcher methods, and the new code explicitly supports off-EDT style changes, so the single slot and dispatcher need one shared synchronization boundary.
Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment.
Codename One is single threaded.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Start-up and per-frame cost, plus three crashes and a stale-artifact bug. Every
measurement below is from a native Mac build; the reasoning behind each change is
in the code rather than here.
Start-up
convertToPixelsasked the main thread which monitor the window is on --through a synchronous dispatch, for every padding and every margin on every
component. The screen and its scale are now published as one value when the
window is built and whenever it moves, so the query reads an atomic.
35ms of blocked event dispatch thread per launch.
AppKitWindowManager's constructor marshalled an observer installsynchronously while the main queue was still bringing AppKit up. Nothing reads
it back, so it no longer waits. 37ms.
monitorEnterannounced a GC park before it knew the lock would block, soeven an uncontended lock waited out the collector's handshake. It now tries the
mutex first. 8.7ms. The handshake loop is also instrumented -- the stall
report could not see it and was reporting zero.
UIManagerscanned the whole theme table once per distinct UIID to answerwhether a dark variant exists. The dark keys are indexed once per theme
generation: first use of a UIID went from 111,955ns to 17,378ns.
Switchbuilt its artwork -- including a gaussian blur -- to answergetPreferredSize, so a switch that is never painted still paid for it duringlayout. The size is derived from the same numbers directly.
Rendering and memory
The Metal image pipeline no longer round-trips every picture through the CPU, and
no longer keeps a decoded copy of each
EncodedImagebeside its GPU texture.Rounded corners are done in the shader instead of building a rounded copy.
Crashes and correctness
longand took the process down.createImageFromARGBImplwas half-merged; its one-pass premultiply is restored.cannot catch this: the symbol simply went missing and the feature shipped inert.
cn1:run,cn1:debug) did not inherit-Dmaven.repo.local, so a child silently resolved from the default repositoryand ran stale artifacts.
Verification
android,ios,mac,javase, the maven plugin andByteCodeTranslatorall buildcore-unittestsregeneratedrather than read stale, since
-amnever reaches it🤖 Generated with Claude Code