-
Notifications
You must be signed in to change notification settings - Fork 64
Avoid the explicit CATransaction
#275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
CATransaction
commitCATransaction
cecb0bc
to
e8ddecc
Compare
use objc2_foundation::{ | ||
ns_string, NSDictionary, NSKeyValueChangeKey, NSKeyValueChangeNewKey, | ||
NSKeyValueObservingOptions, NSNumber, NSObject, NSObjectNSKeyValueObserverRegistration, | ||
NSKeyValueObservingOptions, NSNull, NSNumber, NSObject, NSObjectNSKeyValueObserverRegistration, | ||
NSString, NSValue, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never liked these big mass imports. Any chance we could instead do:
use objc2_foundation as found;
...then do:
found::NSNull
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'd rather avoid that, when reading the implementation, it doesn't actually matter much which framework a specific thing is from - and besides, everything is already prefixed ("NS", "CG" etc.).
I'd rather do:
use objc2_core_foundation::*;
use objc2_core_graphics::*;
use objc2_foundation::*;
use objc2_quartz_core::*;
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I guess objc2_*
could have taken the same approach as cidre
, e.g. cidre::ns::Null
, which is admittedly much more "Rusty", though I decided not to, since it makes it harder to figure out what a specific type corresponds to underneath).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes the code later on harder to read for humans. My vote's still on having a module prefix. I take the same approach in new Win32 code that I write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record, I do something similar in the Windows part of the code.
If you think it's out of scope for this PR, I can file one later.
Wow. This is dramatically faster (up to 1000x !!!) for me. I'm seeing present times measured in 10s of microseconds rather than 10s of milliseconds. Specifically: running Blitz (a winit application) on my 14" MacBook Pro (M1), I'm getting the following for the times to call
To reproduce:
Then:
You should then see output like:
It is the |
e8ddecc
to
91c1904
Compare
Well, to be fair, this PR is not actually doing any work in the The actual work now happens in |
These are expensive, and the layer should be able to figure out the timing of when to render by itself.
91c1904
to
e23a8fb
Compare
Hmm... I wasn't previously timing |
Sorry, that wasn't particularly clear; I meant that
Definitely agree. |
Reading the definition of Or were we accidentally waiting for the transaction to have completed, while this new model allows multiple implicit transactions to be created and submitted "asynchronously"? Just curious to map this to all other platforms' "compositor transaction" abstractions :) |
I think that's true, yeah. Disassembling I suspect that the real problem is actually in the way that Winit schedules redraws such that they happen outside |
Thanks for looking into that! Some quick local testing shows that Could it be that this call is blocking, when applications use it directly? Assigning a completion handler shows that it completes at around the same time. Curious how you "disassembled" so that we can look into Never Note that |
I did
Uhh, pretty sure that's invalid use of the API, otherwise you may be committing work done by something higher in your call stack.
I don't completely know how And yeah, with this PR, you will get a bit of latency here, in that the result is not actually (I'm pretty sure all of these issues just go away if the Winit issue was fixed, since then the |
Apologies, I meant to also skip Curious to see those complete out of order, some frames taking very long. I should've assumed the debugger might have "enough" debug symbols to see what is going on under the hood 😅 And yeah, I've been wanting to write better present-timing abstractions in Winit for years. |
No idea honestly, and unsure of how I'd test it? |
This is what I did, perhaps we could add it to that let s = Instant::now();
unsafe { self.imp.layer.setContents(Some(image.as_ref())) };
static mut FRAME: u32 = 0;
let frame = unsafe { FRAME };
unsafe { FRAME += 1 };
unsafe {
CATransaction::setCompletionBlock(Some(
// Does this clone or otherwise reference the block? After the move,
// the closure lifetime is 'static and could use StackBlock as well?
block2::RcBlock::new(move || {
println!("{frame:0>6}: {:?}", s.elapsed());
})
.deref(),
))
}; |
This PR seems to have a memory problem. I am able to get memory usage to spike as high as 3GB+ with this PR just by scrolling my app (which it causes it to render frames). Interestingly, it does drop if I resize the window, but only down to ~700mb. Rendering this same app with |
Transactions are expensive, and the layer should be able to figure out the timing of when to render by itself (by virtue of being installed in a view). The only reason why we did it before was to avoid a fade transition between layer content changes.
Part of #83. I have not benchmarked this, but I have visibly confirmed less stuttering when resizing.