DaddyFrosty/yoga
1
0
Fork 0
Code Issues 96 Pull Requests 31 Actions Packages Projects Releases 33 Wiki Activity

[csharp,iOS] Fix callbacks on AOT #386

Closed
rmarinho wants to merge 6 commits from fix-logger-xamarinios into master
pull from: fix-logger-xamarinios
merge into: DaddyFrosty:master
Conversation 14 Commits 6 Files Changed 3 +61 -23
rmarinho commented 2017-02-10 08:12:26 -08:00 (Migrated from github.com)
Copy Link

When using AOT mode on Mono one can't use instance methods for callbacks, the compiler needs the MonoPInvokeCallback attribute on a static method to know how to get back to the managed world.
This worked fine without the change in JIT mode.

I not sure if we should use a MONO flag for this stuff as this could be needed for other usage that not only on iOS.

The adicional change is that one should as good practice call Dispose on the YogaNode when using callbacks to make sure we remove the handler from our dictionary, i was trying to write a test for this.. but i saw TestMeasureFuncWithDestructor was already there, but it doesn't seem correct to me, can you check @splhack.

When using AOT mode on Mono one can't use instance methods for callbacks, the compiler needs the MonoPInvokeCallback attribute on a static method to know how to get back to the managed world. This worked fine without the change in JIT mode. I not sure if we should use a __MONO__ flag for this stuff as this could be needed for other usage that not only on iOS. ~~The adicional change is that one should as good practice call Dispose on the YogaNode when using callbacks to make sure we remove the handler from our dictionary, i was trying to write a test for this.. but i saw TestMeasureFuncWithDestructor was already there, but it doesn't seem correct to me, can you check @splhack.~~
splhack commented 2017-02-10 09:08:54 -08:00 (Migrated from github.com)
Copy Link

I'll look into this.

I'll look into this.
rmarinho commented 2017-02-10 11:18:54 -08:00 (Migrated from github.com)
Copy Link

Ok after internal feedback from Alex Corrado i got a new implementation for the fix using the native ref context of Yoga.

Ok after internal feedback from Alex Corrado i got a new implementation for the fix using the native ref context of Yoga.
rmarinho commented 2017-02-10 11:19:25 -08:00 (Migrated from github.com)
Copy Link

The test for the Parent destructor is failing for me on AOT .. still looking at that one.

The test for the Parent destructor is failing for me on AOT .. still looking at that one.
splhack commented 2017-02-10 11:36:05 -08:00 (Migrated from github.com)
Copy Link

can you take a look at #387 ?

can you take a look at #387 ?
chkn commented 2017-02-10 12:17:20 -08:00 (Migrated from github.com)
Copy Link

@splhack It looks like a more complicated way of doing the same thing? I do see one improvement: using a weak GCHandle and only allocating one of them.

Also note that neither PR appears to ever free the GCHandle

@splhack It looks like a more complicated way of doing the same thing? I do see one improvement: using a weak GCHandle and only allocating one of them. Also note that neither PR appears to ever free the GCHandle
splhack commented 2017-02-10 12:59:10 -08:00 (Migrated from github.com)
Copy Link

Ah, I haven't seen the updated diff.

Ah, I haven't seen the updated diff.
splhack commented 2017-02-10 13:56:50 -08:00 (Migrated from github.com)
Copy Link

revised in #388

revised in #388
facebook-github-bot commented 2017-02-10 14:02:56 -08:00 (Migrated from github.com)
Copy Link

@splhack has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@splhack has imported this pull request. If you are a Facebook employee, you can view this diff [on Phabricator](https://phabricator.intern.facebook.com/D4546030).
splhack commented 2017-02-10 15:06:39 -08:00 (Migrated from github.com)
Copy Link

Could you rebase this onto the current master too? I'll merge this and #388

Could you rebase this onto the current master too? I'll merge this and #388
rmarinho commented 2017-02-10 17:07:09 -08:00 (Migrated from github.com)
Copy Link

It's rebased on top of master..

It's rebased on top of master..
facebook-github-bot commented 2017-02-10 17:34:28 -08:00 (Migrated from github.com)
Copy Link

@splhack has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@splhack has imported this pull request. If you are a Facebook employee, you can view this diff [on Phabricator](https://phabricator.intern.facebook.com/D4546030).
rolfbjarne (Migrated from github.com) reviewed 2017-02-13 00:17:04 -08:00
csharp/Facebook.Yoga/YogaLogger.cs
@@ -44,0 +39,4 @@
Logger(level, message);
}
if (level == YogaLogLevel.Error)
rolfbjarne (Migrated from github.com) commented 2017-02-13 00:11:20 -08:00
Copy Link

You need to store the delegate in a static field, otherwise the GC will collect it and the app will crash when the callback is called:

static Func _logger_internal = LoggerInternal;

Native.YGInteropSetLogger (_logger_internal);
You need to store the delegate in a static field, otherwise the GC will collect it and the app will crash when the callback is called: static Func _logger_internal = LoggerInternal; Native.YGInteropSetLogger (_logger_internal);
csharp/Facebook.Yoga/YogaNode.cs
@@ -568,1 +577,3 @@
private YogaSize MeasureInternal(
#if __IOS__
[ObjCRuntime.MonoPInvokeCallback(typeof(YogaMeasureFunc))]
#endif
rolfbjarne (Migrated from github.com) commented 2017-02-13 00:16:55 -08:00
Copy Link

Your patch never calls GCHandle.Free.

Also you allocate a GCHandle both SetMeasureFunction and SetBaselineFunction, if both are called one of the values stored with YGNodeSetContext will be overwritten. The same will happen if either function is called more than once.

Your patch never calls `GCHandle.Free`. Also you allocate a GCHandle both SetMeasureFunction and SetBaselineFunction, if both are called one of the values stored with YGNodeSetContext will be overwritten. The same will happen if either function is called more than once.
rmarinho (Migrated from github.com) reviewed 2017-02-13 02:34:21 -08:00
csharp/Facebook.Yoga/YogaLogger.cs
@@ -44,0 +39,4 @@
Logger(level, message);
}
if (level == YogaLogLevel.Error)
rmarinho (Migrated from github.com) commented 2017-02-13 02:33:47 -08:00
Copy Link

@splhack can you take a note on this in your PR ?

@splhack can you take a note on this in your PR ?
csharp/Facebook.Yoga/YogaNode.cs
@@ -568,1 +577,3 @@
private YogaSize MeasureInternal(
#if __IOS__
[ObjCRuntime.MonoPInvokeCallback(typeof(YogaMeasureFunc))]
#endif
rmarinho (Migrated from github.com) commented 2017-02-13 02:34:15 -08:00
Copy Link

@splhack pr #388 fixes this.

@splhack pr #388 fixes this.
splhack (Migrated from github.com) reviewed 2017-02-13 08:40:37 -08:00
csharp/Facebook.Yoga/YogaLogger.cs
@@ -44,0 +39,4 @@
Logger(level, message);
}
if (level == YogaLogLevel.Error)
splhack (Migrated from github.com) commented 2017-02-13 08:40:37 -08:00
Copy Link

sure

sure

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
Algorithm
Build and Infrastructure
C#
C++ Enhancement
CLA Signed
Discussion
Documentation
Feature Request
Java
JavaScript
Merged
Question
Reverted
Shared with Meta
Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
YogaKit
YogaLayout ViewGroup
bug
dependencies
Pull requests that update a dependency file
duplicate
enhancement
fb-exported
fh:direct-merge-enabled
invalid
ruby
Pull requests that update Ruby code
wontfix
No Label
CLA Signed
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
DaddyFrosty
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#386
No description provided.
Delete Branch "fix-logger-xamarinios"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?