Skip to content

Add internal block type for use in the compiler. IncrementalBuilder only maintains two states - #11750

Merged
dsyme merged 6 commits into
dotnet:mainfrom
TIHan:incr-build-state-refactor
Jul 2, 2021
Merged

Add internal block type for use in the compiler. IncrementalBuilder only maintains two states#11750
dsyme merged 6 commits into
dotnet:mainfrom
TIHan:incr-build-state-refactor

Conversation

@TIHan

@TIHanTIHan commented Jun 29, 2021

Copy link
Copy Markdown
Contributor

This is mostly a refactoring PR. It does add the block type for internal compiler use; it is basically ImmutableArray<'T> but with F# idiomatic collection functions.

Incremental Builder changes:

  • Added IncrementalBuilderInitialState to group the initial state when creating an IncrementalBuilder internally.
  • Lifted a lot of local functions in IncrementalBuilder to be top-level.
  • IncrementalBuilder now manages only two states: IncrementalBuilderInitialState and IncrementalBuilderState.

@TIHanTIHan changed the title Add internal block type for use in the compiler. IncrementalBuilder only maintains two states. Add internal block type for use in the compiler. IncrementalBuilder only maintains two statesJun 29, 2021

@NinoFlorisNinoFloris left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I did a review of the block api, I hope that's alright.

Also, sometimes there are reasons to get the underlying array cheaply. I'm using this, posted below, in our codebase here and there. Though going fully unsafe you could do Unsafe.As<Block<'T>, 'T[]> as it's the only field in the struct. I just prefer my code to die a little nicer than with a segfault in the very unlikely case ImmutableArray changes implementations 😅.

letunsafeGetArray(value:ImmutableArray<'T>)=match MemoryMarshal.TryGetArray(value.AsMemory())with|false,_-> failwith "Expected ImmutableArray to wrap a managed array but it didn't."|true, seg -> seg.Array

Comment threadsrc/fsharp/block.fs Outdated
Comment threadsrc/fsharp/block.fs
Comment threadsrc/fsharp/block.fs Outdated
Comment threadsrc/fsharp/block.fs
Comment threadsrc/fsharp/block.fs Outdated
Comment threadsrc/fsharp/block.fs
Comment threadsrc/fsharp/block.fs
@TIHan

Copy link
Copy Markdown
ContributorAuthor

@NinoFloris of course! Thank you for reviewing :)

@TIHan

TIHan commented Jul 1, 2021

Copy link
Copy Markdown
ContributorAuthor

@NinoFloris This should be a lot better. :) I didn't realize how naïve my original impl was.

@NinoFlorisNinoFloris left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No worries, I picked up a few things since I introduced ImmutableArray into our codebase a few years back. Besides, anything that helps compiler perf is welcome :)

Comment threadsrc/fsharp/block.fs
if result.IsSome then
builder.Add(result.Value)
builder.ToImmutable()
builder.Capacity <- builder.Count

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you don't know the length it doesn't matter too much if you do ToImmutable. You'll do a copy by changing capacity anyway.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Changing the capacity will only copy if the count is different.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fair enough :)

Comment threadsrc/fsharp/block.fs
let builder = ImmutableArray.CreateBuilder(acc)
for i = 0 to arrs.Length - 1 do
builder.AddRange(arrs.[i])
builder.MoveToImmutable()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing capacity <- count or ToImmutable here, as capacity could be anything.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I think this is fine. acc is the total length of the final array.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah I completely missed the the length summing loop, yep it's fine!

Comment threadsrc/fsharp/block.fs
ImmutableArray.Create(item)

let filter predicate (arr: block<'T>) : block<'T> =
let builder = ImmutableArray.CreateBuilder(arr.Length)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Test for empty initially

@dsyme
dsyme merged commit a85aac5 into dotnet:mainJul 2, 2021
Sign up for freeto 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.

3 participants

@TIHan@NinoFloris@dsyme