-
Notifications
You must be signed in to change notification settings - Fork 6k
[Impeller] Create an autorelease pool for Impeller tests running on macOS. #42265
[Impeller] Create an autorelease pool for Impeller tests running on macOS. #42265
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
gaaclarke
left a comment
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.
It seems like we should have an autorelease pool drained after each test, instead of each test suite, no?
| ~ScopedNSAutoreleasePool(); | ||
|
|
||
| private: | ||
| void* autorelease_pool_; |
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.
add FML_DISALLOW_COPY_AND_ASSIGN
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.
done
| #include "flutter/fml/platform/darwin/scoped_nsautorelease_pool.h" | ||
|
|
||
| extern "C" { | ||
| void* objc_autoreleasePoolPush(void); |
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.
These are undocumented methods. Can you just turn this to a .mm file and use the objc objects?
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.
Alternatively, just rename the playground test file to golden_playground_test_mac.mm and create and destroy the pool in the SetUp and TearDown (storing the pool in GoldenPlaygroundTest::GoldenPlaygroundTestImpl).
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.
This needs to be importable into non-objc TUs See https://github.com/flutter/engine/pull/40748/files for example
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.
This needs to be importable into non-objc TUs See https://github.com/flutter/engine/pull/40748/files for example
GoldenPlaygroundTest already handles this by using the pimpl pattern. For PlaygroundTest it can just remain a void* which is no worse than what we have now.
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.
This is based on Chromium's usage of these APIs in a similar class: see https://source.chromium.org/chromium/chromium/src/+/main:base/mac/scoped_nsautorelease_pool.cc
Relying on implementation details of the ObjC runtime is not ideal, but I don't think it will cause issues as long as it's hidden behind the ScopedNSAutoreleasePool abstraction.
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.
Okay, if we keep the undocumented functions can you add this file to a testonly build target? On iOS using undocumented functions can result in getting rejected from the App Store. We want to make sure that it doesn't find it's way into the iOS code.
|
I believe @dnfield already added an autorelease pool utility to the playground harness. Should we replace it with this new FML util? https://github.com/flutter/engine/pull/40748/files |
dnfield
left a comment
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.
LGTM. Consider using this in https://github.com/flutter/engine/blob/main/impeller/playground/playground.cc#L265 as well. The feedback on that change was to move the class to FML.
LGTM
b9ba772 to
f23c718
Compare
The test fixture class containing the autorelease pool will be instantiated separately for each test case. |
Replaced the autorelease wrapper in |
gaaclarke
left a comment
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.
It seems like we should have an autorelease pool drained after each test, instead of each test suite, no?
The test fixture class containing the autorelease pool will be instantiated separately for each test case.
You're right, I got mixed up with other testing libraries. Everything LGTM modulo just using the documented API for autorelease pools. Using undocumented functions is needlessly risky and complex imo.
f23c718 to
587f754
Compare
587f754 to
7ecbfa1
Compare
|
Changed to the implementation based on |
gaaclarke
left a comment
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.
LGTM. I think this is needlessly convoluted in comparison to just using a .mm file but this will work.
7ecbfa1 to
5f26290
Compare
This is specifically needed for TUs where you can't just use a .mm file - if you could you would just do |
You just compile the file you need depending on the platform. You can't use BUILD.gnexecutable("fml") {
source = ["autorelease_pool.h"]
if (is_mac || is_ios) {
sources += ["autorelease_pool_darwin.mm"]
} else {
# Add a stub if you don't want to conditionally include the header.
sources += ["autorelease_pool_stub.cc"]
}
}autorelease_pool.hclass AutoreleasePool {
private:
void* pool_;
};autorelease_pool_darwin.mmAutoreleasePool::AutoreleasePool() {
pool_ = [[NSAutoreleasePool alloc] init];
}
AutoreleasePool::~AutoreleasePool() {
[pool_ drain];
} |
…127536) flutter/engine@c641f63...9ba461e 2023-05-24 [email protected] [web] Remove comment about dart:html migration (flutter/engine#42290) 2023-05-24 [email protected] [web] Hide JS types from dart:ui_web (flutter/engine#42252) 2023-05-24 [email protected] Roll Fuchsia Mac SDK from qoLy9E5PjnAlICjUb... to RSSC61ubl9JXmn4JO... (flutter/engine#42287) 2023-05-24 [email protected] [web] Update a11y announcements to append divs instead of setting content. (flutter/engine#42258) 2023-05-24 [email protected] [Impeller] fix Xcode frame capture. (flutter/engine#42289) 2023-05-24 [email protected] [Impeller] Create an autorelease pool for Impeller tests running on macOS. (flutter/engine#42265) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from qoLy9E5PjnAl to RSSC61ubl9JX If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
See flutter/flutter#127358