Skip to content
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

fix(ios): remove additional gc protection once proxy is remembered #11028

Merged
merged 8 commits into from Oct 14, 2019

Conversation

janvennemann
Copy link
Contributor

@janvennemann janvennemann commented Jul 5, 2019

JIRA: https://jira.appcelerator.org/browse/TIMOB-27207

Optional Description:
To protected JS objects after creation and while they are not referenced on the stack or in the JS object graph we introduced applyGarbageCollectionSafeguard. The GC protection is removed in https://github.com/appcelerator/titanium_mobile/blob/58c26e10d4641d36b7ac7f28fd61fd0ecb9d394e/iphone/TitaniumKit/TitaniumKit/Sources/API/TiBindingTiValue.m#L202 when a value crosses the native bridge. This works fine for statements like const table = Ti.UI.createTableView().

However, this can cause issues for some internal values, like the TiUITableViewSectionProxy of a table view. Unless you explicitly access that section in your code (which will force it to cross the native bridge, e.g. const section = table.data[0]) it will remain GC protected. Since we already make sure that those proxies don't get GC'ed by using rememberProxy, this fix will remove the creation time GC safeguard in this method too.

Testing steps

  • Use the provided example app in the ticket or copy & paste the following simplified example:
    const win = Ti.UI.createWindow({
      layout: 'vertical'
    });
    const table = Ti.UI.createTableView({
      data: [ {title: 'Apples'}, {title: 'Bananas'}, {title: 'Carrots'}, {title: 'Potatoes'} ]
    });
    const btn = Ti.UI.createButton({
      title: 'Reload',
      top: 40
    });
    btn.addEventListener('click', () => {
      table.data = [ {title: 'Apples'}, {title: 'Bananas'}, {title: 'Carrots'}, {title: 'Potatoes'} ];
    });
    win.add(btn);
    win.add(table);
    win.open();
  • Build and run the app. Open Instruments with the Leaks template, attach to the running app and filter for tiui.
  • Click the Reload button multiple times
  • TiUITableViewRowProxy and TiUITableViewSectionProxy entries show up with an increasing number of persistent objects.
  • Wait a short time until the JS garbage collection starts. Most of the object count should move to Transient now.

@janvennemann janvennemann added this to the 8.2.0 milestone Jul 5, 2019
@build build requested a review from a team July 5, 2019 10:27
@build
Copy link
Contributor

build commented Jul 5, 2019

Warnings
⚠️ This PR has milestone set to 8.2.1, but the version defined in package.json is 8.3.0 Please either: - Update the milestone on the PR - Update the version in package.json - Hold the PR to be merged later after a release and version bump on this branch
Messages
📖

💾 Here's the generated SDK zipfile.

📖

✅ All tests are passing
Nice one! All 4122 tests are passing.
(There are 469 skipped tests not included in that total)

📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.

Generated by 🚫 dangerJS against eb11336

Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

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

The code changes look OK to me. I'm going to trust you know the object lifecycle in detail better than I do here, so 👍

@keerthi1032
Copy link
Contributor

keerthi1032 commented Aug 22, 2019

@janvennemann when I tested the pr with the alloy app attached and sample code on the PR . still memory is not released on some devices

  1. iPhone X ios 11.3 device,iphone 8 plus iOS 11.3 simulator.iphone x iOS 12.4 simulator still memory is not released on alloy app provided in the ticket but on classic app with sample code in pr memory is released properly.
  2. on iPhone 5 iOS 10.3 don’t see the fix for both alloy and classic
  3. on iPhone X iOS 11.3. device able to see changes on classic app but not on alloy
  4. on iPhone 6s iOS 12.3 able to see changes on both alloy and classic.works as expected

Screen Shot 2019-08-22 at 2 14 38 PM

[Instruments10.3alloy.trace.zip](https://github.com/appcelerator/titanium_mobile/files/3531971/Instruments10.3alloy.trace.zip) [Instrumentsclassic10.3.trace.zip](https://github.com/appcelerator/titanium_mobile/files/3531972/Instrumentsclassic10.3.trace.zip) [iphonxalloy.trace.zip](https://github.com/appcelerator/titanium_mobile/files/3531978/iphonxalloy.trace.zip)

Adding instrument file for iPhone 5 iOS 10 alloy and classic app and iPhone X alloy trace for reference . Can you please look at it .

@janvennemann
Copy link
Contributor Author

@keerthi1032 i've added a temporary debug function that should make this easier to verify. However, I don't have a device with iOS 11 or lower at hand, so could you please give this another try?

Add the following code to the examples and it will trigger a forced garbage collection after 30 seconds and object count should update.

setTimeout(() => {
  Ti.App.iOS.garbageCollectForDebugging();
}, 30000);

I've tested this with an iPhone X running iOS 12.4 and iPhone 6s running iOS 12.1 and the object count updated as expected for the classic and alloy examples.

@hansemannn
Copy link
Collaborator

hansemannn commented Sep 3, 2019

We still see regular app freezes with this pull request, unfortunately it does not help so far. Even worse, it seems like freezes are not caught as crashes, so it's hard to report from outside.

@sgtcoolguy sgtcoolguy modified the milestones: 8.2.0, 8.2.1 Sep 5, 2019
@lokeshchdhry
Copy link
Contributor

FR Passed.

Objects are getting GC'ed as expected. Some stay persistent for few minutes & then gets GC'ed.

Studio Ver: 5.1.4.201909061933
SDK Ver: 8.3.0 local build
OS Ver: 10.14.5
Xcode Ver: Xcode 11.1
Appc NPM: 4.2.15
Appc CLI: 7.1.1
Daemon Ver: 1.1.3
Ti CLI Ver: 5.2.1
Alloy Ver: 1.14.1
Node Ver: 10.16.1
NPM Ver: 6.9.0
Java Ver: 10.0.2
IOS Devices: ⇨ iPhone 13.1, iphone 12.4

@lokeshchdhry lokeshchdhry merged commit 2ac7d80 into tidev:master Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants