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

Make source type generic #147

Merged
merged 3 commits into from
Jan 5, 2020
Merged

Make source type generic #147

merged 3 commits into from
Jan 5, 2020

Conversation

etaoins
Copy link
Contributor

@etaoins etaoins commented Jan 5, 2020

THIS IS A BREAKING CHANGE

When porting Arret from codespan 0.3 (see etaoins/arret#160) one of the missing features was being able use non-String types for storing source.

Arret has two potential uses for this:

  1. When loading native libraries exposing Arret functions (known as RFI libraries) their Arret type is specified as a per-function&'static str. An example can be seen here:

    https://github.com/etaoins/arret/blob/dfc41bed1c992cc57023359743f68c639d667f25/stdlib/rust/list.rs#L8

    With codespan 0.3 we used Cow<'static str> to avoid allocating a new String for every RFI function by pointing directly inside the loaded library's binary.

  2. Arret is multithreaded but needs to maintain a global Files instance to allocate FileIds. It uses a read-write lock for thread safety and is sensitive to contention on that lock in some circumstances.

    Because we need a FileId in our AST we can't parse the source to AST until we've added it to Files. However, because Files takes ownership of the source we need to hold a read lock on Files to usefiles.source(). The effectively serialises loading source files across all threads.

    Previously we used Arc<FileMap> to drop the lock on CodeMap and then parse the FileMap locklessly. The API doesn't allow this anymore but we could use Source = Arc<str> to similar effect.

Unfortunately the above two uses are mutually exclusive using standard library types. However, either one is an improvement over String and a crate type like supercow could potentially satisfy both uses.

This is a breaking change due to adding a new generic parameter to Files. I initially attempted to use a default of String but this doesn't work for Files::new due to rust-lang/rust#27336. See rust-lang/wg-allocators#1 for an analogous case to this.

**THIS IS A BREAKING CHANGE**

When porting Arret from `codespan` 0.3 (see etaoins/arret#160) one of
the missing features was being able use non-`String` types for storing
source.

Arret has two potential uses for this:

1. When loading native libraries exposing Arret functions (known as RFI
   libraries) their Arret type is specified as a per-function
   `&'static str`. An example can be seen here:

   https://github.com/etaoins/arret/blob/dfc41bed1c992cc57023359743f68c639d667f25/stdlib/rust/list.rs#L8

   With `codespan` 0.3 we used `Cow<'static str>` to avoid allocating a
   new `String` for every RFI function by pointing directly inside the
   loaded library's binary.

2. Arret is multithreaded but needs to maintain a global `Files`
   instance to allocate `FileId`s. It uses a read-write lock for thread
   safety and is sensitive to contention on that lock in some
   circumstances.

   Because we need a `FileId` in our AST we can't parse the source to
   AST until we've added it to `Files`. However, because `Files` takes
   ownership of the source we need to hold a read lock on `Files` to use
   `files.source()`. The effectively serialises loading source files
   across all threads.

   Previously we used `Arc<FileMap>` to drop the lock on `CodeMap` and
   then parse the `FileMap` locklessly. The API doesn't allow this
   anymore but we could use `Source = Arc<str>` to similar effect.

Unfortunately the above two uses are mutually exclusive using standard
library types. However, either one is an improvement over `String` and a
crate type like `supercow` could potentially satisfy both uses.

This is a breaking change due to adding a new generic parameter to
`Files`. I initially attempted to use a default of `String` but this
doesn't work for `Files::new` due to rust-lang/rust#27336. See
rust-lang/wg-allocators#1 for an analogous case to this.
This ports `codespan-reporting` and `codespan-lsp` to use `Files`'s new
`Source` generic parameter.
@brendanzab
Copy link
Owner

brendanzab commented Jan 5, 2020

This looks good! I think I broke it with one of the merges - but it might be nice to get this into 0.7 - see #142.

@etaoins
Copy link
Contributor Author

etaoins commented Jan 5, 2020

No worries, I’ll rebase this when I get home 👍

@etaoins
Copy link
Contributor Author

etaoins commented Jan 5, 2020

@brendanzab Updated!

@brendanzab brendanzab merged commit 92229c1 into brendanzab:master Jan 5, 2020
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.

2 participants