use fallback for MemoryMappedFile.CreateOrOpen when not running on windows #270

hahn-kev · 2023-01-05T08:35:38Z

Creating this PR to fix an issue seen here: sillsdev/LfMerge#317

open to suggestions, maybe remove the IsMono check? But that might cause other issues, possibly for mono running on windows.

This change is

…ndows

megahirt · 2023-01-05T08:41:53Z

@hahn-kev and I looked at this together in response to two users whose projects fail to S/R on language forge because of an exception in liblcm.

I am in favor of changing the if statement to simply

if (!MiscUtils.IsWindows)

since running Mono on Windows isn't an use case for liblcm as far as I understand.

megahirt · 2023-01-05T08:50:31Z

Looking at this code has me wondering if we can simply always use the platform agnostic code path of MemoryMappedFile.CreateFromFile(name) in all cases including Windows, but I don't understand if that would change behavior in any way on Windows.

papeh · 2023-01-05T14:45:58Z

src/SIL.LCModel/Infrastructure/Impl/SharedXMLBackendProvider.cs

 		private MemoryMappedFile CreateOrOpen(string name, long capacity, bool createdNew)
 		{
-			if (MiscUtils.IsMono)
+			if (MiscUtils.IsMono || !MiscUtils.IsWindows)


Would SIL.PlatformUtilities.Platform.IsUnix (alone) work here? If not, !Platform.IsWindows alone should work. Both are in SIL.Core. Either of these

I'm not sure, I didn't know if Unix would work for both Linux and Mac (or something else). However the way the failing API is written is that it only works for Windows, so this seemed the way to go instead of something else as the API is only supported in windows, rather than not being supported in Unix. Though that may end up being the same thing at the end of the day.

papeh

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @hahn-kev)

src/SIL.LCModel/Infrastructure/Impl/SharedXMLBackendProvider.cs line 265 at r1 (raw file):

Previously, hahn-kev (Kevin Hahn) wrote…

I'm not sure, I didn't know if Unix would work for both Linux and Mac (or something else). However the way the failing API is written is that it only works for Windows, so this seemed the way to go instead of something else as the API is only supported in windows, rather than not being supported in Unix. Though that may end up being the same thing at the end of the day.

When this was originally written, Mono, Linux, and Unix were effectively synonymous, and Windows was the only other choice. If you think checking for Windows instead of Unix would be more future-proof, we can check for Windows instead.

src/SIL.LCModel/Infrastructure/Impl/SharedXMLBackendProvider.cs line 272 at r1 (raw file):

					File.Delete(name);

				// Mono only supports memory mapped files that are backed by an actual file

Per the discussion, this seems to be a problem with Unix rather than Mono

Suggestion:

Unix

megahirt

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @papeh)

src/SIL.LCModel/Infrastructure/Impl/SharedXMLBackendProvider.cs line 265 at r1 (raw file):

Previously, papeh wrote…

When this was originally written, Mono, Linux, and Unix were effectively synonymous, and Windows was the only other choice. If you think checking for Windows instead of Unix would be more future-proof, we can check for Windows instead.

If we have to keep an if statement, then I would prefer
if (!SIL.PlatformUtilities.Platform.IsWindows)

@papeh what do you think about ditching the named memory mapped files entirely (a windows-specific feature) and go with the implementation as coded on all platforms, including windows? I know we'd have to test it somehow (does the program run?) but I think it would be reduced complexity to simply do the same on all platforms.

src/SIL.LCModel/Infrastructure/Impl/SharedXMLBackendProvider.cs line 272 at r1 (raw file):

Previously, papeh wrote…

Per the discussion, this seems to be a problem with Unix rather than Mono

It is a issue with Unix/Linux systems, not Mono per say, but the comment is basically referring to the only use-case which is Mono on Linux.

papeh

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @hahn-kev and @megahirt)

src/SIL.LCModel/Infrastructure/Impl/SharedXMLBackendProvider.cs line 265 at r1 (raw file):

Previously, megahirt (Christopher Hirt) wrote…

If we have to keep an if statement, then I would prefer
if (!SIL.PlatformUtilities.Platform.IsWindows)

@papeh what do you think about ditching the named memory mapped files entirely (a windows-specific feature) and go with the implementation as coded on all platforms, including windows? I know we'd have to test it somehow (does the program run?) but I think it would be reduced complexity to simply do the same on all platforms.

That sounds reasonable.

papeh

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @hahn-kev and @megahirt)

src/SIL.LCModel/Infrastructure/Impl/SharedXMLBackendProvider.cs line 265 at r1 (raw file):

Previously, papeh wrote…

That sounds reasonable.

Although I wonder how the file-backed file would affect performance.

megahirt

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jasonleenaylor and @papeh)

src/SIL.LCModel/Infrastructure/Impl/SharedXMLBackendProvider.cs line 265 at r1 (raw file):

Previously, papeh wrote…

Although I wonder how the file-backed file would affect performance.

Yeah I wondered about that too, however I realistically don't know how to test the performance, or even trigger this code path (could take some guesses). @jasonleenaylor Do you have an opinion here?

Just from the class name MemoryMappedFile I assume we are always dealing with the filesystem, even if Windows has an extra api method that abstracts the file into just a name without a path. So that makes me think the Windows-specific method is doing something similar to what we are doing in our code. Can we look at the source of how MemoryMappedFile.CreateOrOpen(name, capacity) is implemented to see if it's similar to our own implementation?

papeh

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jasonleenaylor)

src/SIL.LCModel/Infrastructure/Impl/SharedXMLBackendProvider.cs line 265 at r1 (raw file):

Previously, megahirt (Christopher Hirt) wrote…

Yeah I wondered about that too, however I realistically don't know how to test the performance, or even trigger this code path (could take some guesses). @jasonleenaylor Do you have an opinion here?

Just from the class name MemoryMappedFile I assume we are always dealing with the filesystem, even if Windows has an extra api method that abstracts the file into just a name without a path. So that makes me think the Windows-specific method is doing something similar to what we are doing in our code. Can we look at the source of how MemoryMappedFile.CreateOrOpen(name, capacity) is implemented to see if it's similar to our own implementation?

Resharper VS plugin would allow viewing of decompiled MS sources. If neither of you have a license, I could take a look sometime.

hahn-kev · 2023-01-10T02:30:03Z

it looks like the rough logic of the windows API is to just create the mapped file, and if that fails try to open one instead. If that fails then it tries again 14 times, if it still fails then it throws an error. Otherwise it'll return.

Our current logic basically deletes the file (if it exists and creating a new one was requested), then if the file doesn't exist (because we deleted it or it never existed) we create it , then just return the memory mapped file.

The biggest difference that I see (other then the whole retry functionality) is that we will delete an old file if we want to create a new one, where as the windows API will never do that. It's not clear to me why the API can't be ported to work on other platforms so there's probably something I'm missing with my understanding of the APIs.

ermshiperete · 2023-01-10T09:07:27Z

In my understanding, on Windows a memory mapped file can be persisted or non-persisted. Only the former type uses a file on disk, in the latter case it's just shared memory. The .NET implementation directly uses the Windows system APIs. On Linux a memory mapped file always has a corresponding file on disk, IIRC. For the non-persistent type you'd have to implement it with a different system API. With that API it might be way harder to implement the functionality of MemoryMappedFile.

megahirt · 2023-01-10T10:41:40Z

In my understanding, on Windows a memory mapped file can be persisted or non-persisted. Only the former type uses a file on disk, in the latter case it's just shared memory. The .NET implementation directly uses the Windows system APIs. On Linux a memory mapped file always has a corresponding file on disk, IIRC. For the non-persistent type you'd have to implement it with a different system API. With that API it might be way harder to implement the functionality of MemoryMappedFile.

Thanks for that, Eberhard. In your opinion, would it be worth standardizing on the same implementation (persistent memory-mapped file) across all platforms? Is there somehow a performance hit to using a file-based memory mapped file?

jasonleenaylor

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @papeh)

src/SIL.LCModel/Infrastructure/Impl/SharedXMLBackendProvider.cs line 265 at r1 (raw file):

Previously, papeh wrote…

Resharper VS plugin would allow viewing of decompiled MS sources. If neither of you have a license, I could take a look sometime.

I would prefer that we leave the windows implementation alone. The SharedXMLBackendProvider is used as the storage for the project when the project settings specify it, or if there is Scripture content present in the project. I added a comment about this into the LFMerge issue. Platform.IsUnix is appropriate for this code, it should work on all Unix like platforms..

ermshiperete · 2023-01-11T07:51:36Z

Is there somehow a performance hit to using a file-based memory mapped file?

My gut feeling is that it would have a performance impact since it hits the file system, but I have no idea how much of a difference it would make.

I'd agree with @jasonleenaylor that we shouldn't change the Windows implementation.

megahirt

@hahn-kev Let's get this line changed (suggestion provided) and then get this merged so that we can attempt to move this toward a user fix.

src/SIL.LCModel/Infrastructure/Impl/SharedXMLBackendProvider.cs

megahirt · 2023-01-11T12:10:11Z

I'd agree with @jasonleenaylor that we shouldn't change the Windows implementation.

Thank you Jason and Eberhard for weighing in on this. We'll go with the smallest possible change as you suggested and leave the Windows implementation alone.

…x check as mono is no longer relevant Co-authored-by: Christopher Hirt <[email protected]>

megahirt

Thanks @hahn-kev

megahirt

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ermshiperete and @papeh)

ermshiperete

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @papeh)

papeh

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @hahn-kev and @jasonleenaylor)

src/SIL.LCModel/Infrastructure/Impl/SharedXMLBackendProvider.cs line 265 at r1 (raw file):

Previously, megahirt (Christopher Hirt) wrote…

Based on further conversation with @jasonleenaylor and @ermshiperete we will leave the Windows implementation alone and stick with a simple check for "isUnix":
			if (SIL.PlatformUtilities.Platform.IsUnix)

MiscUtils.IsUnix is deprecated. Please use Platform.IsUnix.

src/SIL.LCModel/Infrastructure/Impl/SharedXMLBackendProvider.cs line 272 at r1 (raw file):

Previously, megahirt (Christopher Hirt) wrote…

It is a issue with Unix/Linux systems, not Mono per say, but the comment is basically referring to the only use-case which is Mono on Linux.

That was the case when this comment was written, but isn't this pull request addressing a problem with running on Unix without Mono?

papeh

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @hahn-kev)

…ndows (#270) * change CreateOrOpen memory-mapped file fallback condition to use a Unix check as Mono is no longer the only way this runs on Linux Co-authored-by: Christopher Hirt <[email protected]> Co-authored-by: papeh <[email protected]>

use fallback for MemoryMappedFile.CreateOrOpen when not running on wi…

2090c03

…ndows

papeh reviewed Jan 5, 2023

View reviewed changes

papeh requested changes Jan 5, 2023

View reviewed changes

megahirt reviewed Jan 5, 2023

View reviewed changes

papeh reviewed Jan 5, 2023

View reviewed changes

megahirt reviewed Jan 6, 2023

View reviewed changes

papeh requested changes Jan 9, 2023

View reviewed changes

jasonleenaylor reviewed Jan 10, 2023

View reviewed changes

megahirt requested changes Jan 11, 2023

View reviewed changes

src/SIL.LCModel/Infrastructure/Impl/SharedXMLBackendProvider.cs Outdated Show resolved Hide resolved

hahn-kev and others added 2 commits January 12, 2023 09:40

change CreateOrOpen memory maped file fallback condition to use a Uni…

80b1ba1

…x check as mono is no longer relevant Co-authored-by: Christopher Hirt <[email protected]>

use correct class for IsUnix check

c159538

megahirt approved these changes Jan 12, 2023

View reviewed changes

megahirt enabled auto-merge January 12, 2023 05:53

megahirt approved these changes Jan 12, 2023

View reviewed changes

megahirt disabled auto-merge January 12, 2023 05:59

ermshiperete requested a review from papeh January 12, 2023 09:44

ermshiperete approved these changes Jan 12, 2023

View reviewed changes

papeh requested changes Jan 12, 2023

View reviewed changes

papeh approved these changes Jan 12, 2023

View reviewed changes

papeh added 2 commits January 12, 2023 13:49

Merge branch 'master' into fix-memory-map-dotnet-core

c6cf08a

Remove references to deprecated MiscUtils

f3cab53

papeh approved these changes Jan 12, 2023

View reviewed changes

papeh merged commit a177e1d into sillsdev:master Jan 12, 2023

hahn-kev mentioned this pull request Jan 16, 2023

use fallback for MemoryMappedFile.CreateOrOpen when not running on windows #272

Merged

hahn-kev mentioned this pull request Apr 21, 2023

System.ArgumentNullException: Value cannot be null sillsdev/LfMerge#323

Closed

hahn-kev mentioned this pull request Sep 11, 2023

Fix create memory mapped linux #287

Merged

Uh oh!

use fallback for MemoryMappedFile.CreateOrOpen when not running on windows #270

use fallback for MemoryMappedFile.CreateOrOpen when not running on windows #270

Uh oh!

Conversation

hahn-kev commented Jan 5, 2023 • edited by ddaspit Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

megahirt commented Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

megahirt commented Jan 5, 2023

Uh oh!

papeh Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

hahn-kev Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

papeh left a comment

Choose a reason for hiding this comment

Uh oh!

megahirt left a comment

Choose a reason for hiding this comment

Uh oh!

papeh left a comment

Choose a reason for hiding this comment

Uh oh!

papeh left a comment

Choose a reason for hiding this comment

Uh oh!

megahirt left a comment

Choose a reason for hiding this comment

Uh oh!

papeh left a comment

Choose a reason for hiding this comment

Uh oh!

hahn-kev commented Jan 10, 2023

Uh oh!

ermshiperete commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

megahirt commented Jan 10, 2023

Uh oh!

jasonleenaylor left a comment

Choose a reason for hiding this comment

Uh oh!

ermshiperete commented Jan 11, 2023

Uh oh!

megahirt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

megahirt commented Jan 11, 2023

Uh oh!

megahirt left a comment

Choose a reason for hiding this comment

Uh oh!

megahirt left a comment

Choose a reason for hiding this comment

Uh oh!

ermshiperete left a comment

Choose a reason for hiding this comment

Uh oh!

papeh left a comment

Choose a reason for hiding this comment

Uh oh!

papeh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hahn-kev commented Jan 5, 2023 •

edited by ddaspit

Loading

megahirt commented Jan 5, 2023 •

edited

Loading

ermshiperete commented Jan 10, 2023 •

edited

Loading