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

Allow BatchNodes to reuse prior computed arrays if they would generate hte same value #62169

Merged
merged 21 commits into from
Jun 30, 2022
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 76 additions & 18 deletions src/Compilers/Core/Portable/SourceGeneration/Nodes/BatchNode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Collections;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
Expand All @@ -30,6 +28,81 @@ public BatchNode(IIncrementalGeneratorNode<TInput> sourceNode, IEqualityComparer

public IIncrementalGeneratorNode<ImmutableArray<TInput>> WithTrackingName(string name) => new BatchNode<TInput>(_sourceNode, _comparer, name);

private (ImmutableArray<TInput>, ImmutableArray<(IncrementalGeneratorRunStep InputStep, int OutputIndex)>) GetValuesAndInputs(
NodeStateTable<TInput> sourceTable,
NodeStateTable<ImmutableArray<TInput>> previousTable,
NodeStateTable<ImmutableArray<TInput>>.Builder newTable)
{
// Do an initial pass to both get the steps, and determine how many entries we'll have.
var sourceInputsBuilder = newTable.TrackIncrementalSteps ? ArrayBuilder<(IncrementalGeneratorRunStep InputStep, int OutputIndex)>.GetInstance() : null;
Copy link
Member

Choose a reason for hiding this comment

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

Could we initialize this with sourceTable.Count as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. that would be a reasonable approximation. The only reasons i didn't were that:

  1. it's not exactly correct. Tables have entries, and entries have steps. So it's quite possible to have way more steps tahn .Count returns.
  2. TrackingIncrementalSteps is done in tests, so being that efficient is not a pressing concern on my part. It doesn't show up in my benchmarks either as i'm not turning on that capability :)

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough.


var entryCount = 0;
foreach (var entry in sourceTable)
{
// Always keep track of its step information, regardless of if the entry was removed or not, so we
// can accurately report how long it took and what actually happened (for testing validation).
sourceInputsBuilder?.Add((entry.Step!, entry.OutputIndex));

if (entry.State != EntryState.Removed)
entryCount++;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

first pass determiens how many entries we have. note: this will allocate an IEnumerable. If we worry about the cost of that, we could have a dedicated method on SourceTable (called something like "CountOfNonRemovedEntries").


var sourceInputs = sourceInputsBuilder != null ? sourceInputsBuilder.ToImmutableAndFree() : default;

// First, see if we can reuse the entries from previousTable.
// If not, produce the actual values we need from sourceTable.
var result = tryReusePreviousTableValues(entryCount) ?? computeCurrentTableValues(entryCount);
return (result, sourceInputs);

ImmutableArray<TInput>? tryReusePreviousTableValues(int entryCount)
{
if (previousTable.Count != 1)
return null;

var previousItems = previousTable.Single().item;

// If they don't have the same length, we clearly can't reuse them.
if (previousItems.Length != entryCount)
return null;

var indexInPrevious = 0;
foreach (var entry in sourceTable)
{
if (entry.State == EntryState.Removed)
continue;

// If the entries aren't the same, we can't reuse.
if (!EqualityComparer<TInput>.Default.Equals(entry.Item, previousItems[indexInPrevious]))
return null;

indexInPrevious++;
}

// We better have the exact same count as previousItems as we checked that above.
Debug.Assert(indexInPrevious == previousItems.Length);

// Looks good, we can reuse this.
return previousItems;
}

ImmutableArray<TInput> computeCurrentTableValues(int entryCount)
Copy link
Member Author

Choose a reason for hiding this comment

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

finally, we do a pass where we allocate a builder. however, importantly, we know the exact amount of items we need, so the builder doesn't create a scratch array that has the chance of not being pooled if too large.

{
// Important: we initialize with the exact capacity we need here so that we don't make a pointless
// scratch array that may be very large and may cause GC churn when it cannot be returned to the pool.
var builder = ArrayBuilder<TInput>.GetInstance(entryCount);
foreach (var entry in sourceTable)
{
if (entry.State == EntryState.Removed)
continue;

builder.Add(entry.Item);
}

Debug.Assert(builder.Count == entryCount);
return builder.ToImmutableAndFree();
}
}

public NodeStateTable<ImmutableArray<TInput>> UpdateStateTable(DriverStateTable.Builder builder, NodeStateTable<ImmutableArray<TInput>> previousTable, CancellationToken cancellationToken)
{
// grab the source inputs
Expand All @@ -50,22 +123,7 @@ public NodeStateTable<ImmutableArray<TInput>> UpdateStateTable(DriverStateTable.

var stopwatch = SharedStopwatch.StartNew();

var sourceValuesBuilder = ArrayBuilder<TInput>.GetInstance();
var sourceInputsBuilder = newTable.TrackIncrementalSteps ? ArrayBuilder<(IncrementalGeneratorRunStep InputStep, int OutputIndex)>.GetInstance() : null;

foreach (var entry in sourceTable)
{
// At this point, we can remove any 'Removed' items and ensure they're not in our list of states.
if (entry.State != EntryState.Removed)
sourceValuesBuilder.Add(entry.Item);

// However, regardless of if the entry was removed or not, we still keep track of its step information
// so we can accurately report how long it took and what actually happened (for testing validation).
sourceInputsBuilder?.Add((entry.Step!, entry.OutputIndex));
}

var sourceValues = sourceValuesBuilder.ToImmutableAndFree();
var sourceInputs = newTable.TrackIncrementalSteps ? sourceInputsBuilder!.ToImmutableAndFree() : default;
var (sourceValues, sourceInputs) = GetValuesAndInputs(sourceTable, previousTable, newTable);

if (previousTable.IsEmpty)
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
{
Expand Down