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

try to solve android arch #42

Merged
merged 3 commits into from
Jul 16, 2021
Merged

try to solve android arch #42

merged 3 commits into from
Jul 16, 2021

Conversation

Milerius
Copy link
Contributor

@Milerius Milerius commented Jul 15, 2021

cc @kpango trying to fix #41 #31

@mratsim
Copy link

mratsim commented Jul 15, 2021

Doesn't go have something similar to C++ alignas? https://en.cppreference.com/w/cpp/language/alignas

I'm surprised Go atomics aren't aligned automatically to the underlying size.

Assuming Go atomatically aligns trivial types to their size, yes this will ensure 64-bit alignment.

However from a performance point of view, you might want to separate atomics by about a cacheline (64-byte) for those accessed by multiple threads to avoid false sharing.

@Milerius
Copy link
Contributor Author

Doesn't go have something similar to C++ alignas? https://en.cppreference.com/w/cpp/language/alignas

I'm surprised Go atomics aren't aligned automatically to the underlying size.

Assuming Go atomatically aligns trivial types to their size, yes this will ensure 64-bit alignment.

However from a performance point of view, you might want to separate atomics by about a cacheline (64-byte) for those accessed by multiple threads to avoid false sharing.

https://golang.org/pkg/sync/atomic/#pkg-note-BUG it seems that's is a known bug

@kpango
Copy link
Owner

kpango commented Jul 16, 2021

@Milerius Thank you for the PR. I would love to incorporate this fix if it solves the long standing problem .
Have you tested this on an Android device?
I don't have an Android device on hand, so I can't test it.

fastime.go Outdated
Comment on lines 14 to 25
dur int64
ut int64
unt int64
uut uint32
uunt uint32
running *atomic.Value
t *atomic.Value
ft *atomic.Value
format *atomic.Value
cancel context.CancelFunc
correctionDur time.Duration
dur int64

Copy link
Owner

Choose a reason for hiding this comment

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

time.Duration is also int64 type I think it's better to write like this

	uut           uint32
	uunt          uint32
	ut            int64
	unt           int64
	dur           int64
	correctionDur time.Duration
	running       *atomic.Value
	t             *atomic.Value
	ft            *atomic.Value
	format        *atomic.Value
	cancel        context.CancelFunc

@Milerius
Copy link
Contributor Author

@Milerius Thank you for the PR. I would love to incorporate this fix if it solves the long standing problem .
Have you tested this on an Android device?
I don't have an Android device on hand, so I can't test it.

Hmm I would like an advice how to use this patched version of glg in my program to test my fix also kpango/glg#107 seems to break android compatibility, good things is you can test this local fix by compiling for ARM and use qemu to launch example, but for android i need a way to build glg with my patch

@Milerius
Copy link
Contributor Author

Milerius commented Jul 16, 2021

Something you can try @kpango is:

install android studio.

Compile your module with gomobile bind -v --target=android .

and import it in android studio as mentionned in: https://medium.com/@LogPacker/gomobile-library-development-for-ios-android-8711e76817d8

You do not need any android device because there is a simulator

https://go101.org/article/memory-layout.html

@kpango
Copy link
Owner

kpango commented Jul 16, 2021

Thanks for providing the verification method, I guess I need to release it for Dependency to try it on Android, I'll keep the Issue and merge this PR and release Patch for glg as well.
I'll contact you about it, can you verify it for me?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

panic at runtime on android
3 participants