Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@jason-simmons
Copy link
Member

@flutter-dashboard
Copy link

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.

Copy link
Member

@gaaclarke gaaclarke left a 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_;
Copy link
Member

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

Copy link
Member Author

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);
Copy link
Member

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?

Copy link
Member

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

@gaaclarke gaaclarke May 24, 2023

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.

@bdero
Copy link
Member

bdero commented May 23, 2023

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

Copy link
Contributor

@dnfield dnfield left a 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

@jason-simmons jason-simmons force-pushed the iplr_mac_test_autorelease branch from b9ba772 to f23c718 Compare May 23, 2023 23:42
@jason-simmons
Copy link
Member Author

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.

@jason-simmons
Copy link
Member Author

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.

Replaced the autorelease wrapper in Playground with fml::ScopedNSAutoreleasePool

Copy link
Member

@gaaclarke gaaclarke left a 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.

@jason-simmons jason-simmons force-pushed the iplr_mac_test_autorelease branch from f23c718 to 587f754 Compare May 24, 2023 00:27
@chinmaygarde chinmaygarde changed the title [Impeller] Create an autorelease pool for Impeller tests running on macOS [Impeller] Create an autorelease pool for Impeller tests running on macOS. May 24, 2023
@jason-simmons jason-simmons force-pushed the iplr_mac_test_autorelease branch from 587f754 to 7ecbfa1 Compare May 24, 2023 17:25
@jason-simmons
Copy link
Member Author

Changed to the implementation based on objc_msgSend that had been used in Playground - PTAL

Copy link
Member

@gaaclarke gaaclarke left a 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.

@jason-simmons jason-simmons force-pushed the iplr_mac_test_autorelease branch from 7ecbfa1 to 5f26290 Compare May 24, 2023 18:24
@dnfield
Copy link
Contributor

dnfield commented May 24, 2023

LGTM. I think this is needlessly convoluted in comparison to just using a .mm file but this will work.

This is specifically needed for TUs where you can't just use a .mm file - if you could you would just do @autoreleasepool {} :)

@gaaclarke
Copy link
Member

gaaclarke commented May 24, 2023

This is specifically needed for TUs where you can't just use a .mm file - if you could you would just do @autoreleasepool {} :)

You just compile the file you need depending on the platform. You can't use @autoreleasepool because the code where the scope of the pool isnt in objc++. Is there something I'm missing?

BUILD.gn

executable("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.h

class AutoreleasePool {
  private:
   void* pool_;
};

autorelease_pool_darwin.mm

AutoreleasePool::AutoreleasePool() {
  pool_ = [[NSAutoreleasePool alloc] init];
}

AutoreleasePool::~AutoreleasePool() {
  [pool_ drain];
}

@jason-simmons jason-simmons added the autosubmit Merge PR when tree becomes green via auto submit App label May 24, 2023
@auto-submit auto-submit bot merged commit df37f0c into flutter:main May 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 24, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request May 24, 2023
…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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App e: impeller needs tests

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants