This is only called from a single place after we took them out from collection that linked to the current file. So we are sure those belong to the current file. So this is just unnecessary overhead.

I am not sure why IsScriptBreakpoint was checked here, but it was not re-checked anywhere else. SetPendingBreakpoints below is called without the list of breakpoints to set, and internally it only checks the filepath from the function context. So I skipped the check to avoid looping in case there are thousands of breakpoints in one file.

This is the meat of the improvement when looking up BP, instead of looping over all in the file we get it from the map based on sequence point.

Do you say it is a single place where we get a performance improvement?
If so I wonder did you try to unroll the Linq?

This is not the single place where you get the improvement. The improvement is caused by:

1. Saving the breakpoints mapped by path, and then by sequence point, because that is how the break points are queried. This avoids unnecessary looping.
2. Not moving breakpoints into a new dictionary every time we inspect them. This avoids unnecessary array allocation. https://github.com/PowerShell/PowerShell/pull/14953/files#diff-0a4e4bd42dcf35b5e74e88bce4adba02f6d6f823b698647e3ee706d007b1915bL2051

How would you want this solved? This should happen rarely so I might lock here. Or just keep it as is and don't clean up the dictionary of files.

I don't fully understand the problem we're trying to solve here, but if you/@PaulHigin is able to explain it to me, I can try to weigh in

This is pending breakpoints:

_pendingBreakpoints = new ConcurrentDictionary<string, ConcurrentDictionary<int, LineBreakpoint>>()

Pending breakpoints is a dictionary keyed by filepath, that contains a dictionary keyed by sequence point. When all pending breakpoints were bound, it would be nice to remove the key from the _pendingBreakpoints dictionary.

Something like this:

if (_pendingBreakpoints.TryGetValue(currentScriptFile, out var bpsInThisScript) && bpsInThisScript.IsEmpty) {
    _pendingBreakpoints.TryRemoveValue(currentScriptFile, out _);
}

Unfortunately the first line is not atomic, so there is a race condition between the first line and the second. If someone added breakpoint right after we checked the count, in theory we could lose breakpoints.

This seems like a rare condition and can be solved in few ways.

What I did here is that I simply leave the key in the dictionary. This means 1 extra string + empty concurrent dictionary is left in memory, for each file that had breakpoints. I am guessing there are rarely more than 100 distinct files with breakpoints per powershell session, so this seems okay-ish. But still dirty.

The race condition seems very rare, and simply checking if we removed a dictionary in which an item was added and they try to merge it back in might reduce the possibility of removing a breakpoint even further. We would add another race condition, but the possibility of timing both of them exactly right seems infinitely small. Something like this:

if (_pendingBreakpoints.TryGetValue(currentScriptFile, out var bpsInThisScript) && bpsInThisScript.IsEmpty) {
    if (_pendingBreakpoints.TryRemoveValue(currentScriptFile, out var removedBps) && !removedBps.IsEmpty) {
        // someone added a breakpoint after we counted but before we removed
        // merge it back into _pendingBreakpoints
        // this would happen extremely rarely
    }
}

The race condition needs to be better defined here. How is setting a breakpoint subject to a race here, via API? PowerShell scripts run on a single thread.

That is why I was asking how you want this solved because I don't know enough about the interaction of pending breakpoints and code execution. Maybe there is no way that pending breakpoints collection would be changed while this code is running, because it all runs on a single thread, or maybe adding a breakpoint in vscode UI calls into the PowerShell process and sets the breakpoint from a different thread.

I just assumed it is the latter, which is why ConcurrentDictionary was used in the original code and also in the new code.

I would change the var here

Also please rename bps to something like breakpoints

I know it's just a style thing and it's not your fault @nohwnd, but I don't love this double count check.

Ideally we could just put the check inside TriggerBreakpoints.

If I were really trying to make this suit my desired style, I'd make them into extension methods:

breakpoints.Trigger().StopOnSequencePoint(functionContext);

I don't love it either, but it prevents creating another list in TriggerBreakpoints when we call it with empty list of breakpoints. And moving the check into TriggerBreakpoints makes this code path less obvious.

I also don't love that the breakpoints variable is reused, but I went for the minimal amount of changes in this PR. If you insist on changing it I can do it. As you say it's style related. Should I make the change?

I think it's fine as is for now

I don't fully understand the problem we're trying to solve here, but if you/@PaulHigin is able to explain it to me, I can try to weigh in