ForEach-Object: Wat

Someone mentioned something curious about ForEach-Object and I went to take a look at it.

What the heck is going on in there? Do we really have known and supported script patterns like this?

$val | ForEach-Object { <# begin #> } { <# process 1 #> } { <# process 2 #> } { <# process 3 #> } { <# end #> }

Is there some history here that influences this? Do we really need to tax the parameter binder trying to bind n script blocks?

PowerShell/src/System.Management.Automation/engine/InternalCommands.cs

Lines 103 to 120 in 759c4ab

    
           [Parameter(Mandatory = true, Position = 0, ParameterSetName = "ScriptBlockSet")] 
        
           [AllowNull] 
        
           [AllowEmptyCollection] 
        
           public ScriptBlock[] Process 
        
           { 
        
               set 
        
               { 
        
                   if (value == null) 
        
                       _scripts.Add(null); 
        
                   else 
        
                       _scripts.AddRange(value); 
        
               } 
        
               get 
        
               { 
        
                   return null; 
        
               } 
        
           }

Do we really need -RemainingScripts? I'd really love to hear if there's a rationale behind it I'm missing; I can't really see a whole lot of reasons to allow this kind of input myself.

PowerShell/src/System.Management.Automation/engine/InternalCommands.cs

Lines 145 to 160 in 759c4ab

    
           [SuppressMessage("Microsoft.Performance", "CA1819:PropertiesShouldNotReturnArrays", Justification = "Cmdlets use arrays for parameters.")] 
        
           [Parameter(ParameterSetName = "ScriptBlockSet", ValueFromRemainingArguments = true)] 
        
           [AllowNull] 
        
           [AllowEmptyCollection] 
        
           public ScriptBlock[] RemainingScripts 
        
           { 
        
               set 
        
               { 
        
                   if (value == null) 
        
                       _scripts.Add(null); 
        
                   else 
        
                       _scripts.AddRange(value); 
        
               } 
        
               get { return null; } 
        
           }

Proposal

I don't know all that much about the parameter binder, but it seems to me like this would be significantly more effective with a small handful of parameter sets, and 3 script block inputs, maximum.

ForEach-Object is considered really slow by many advanced users, and I can't help but think that this is a fairly large part of it.

Can we simplify this parameter binding by adding a couple of sets? (Don't think there's a whole lot of utility allowing -Process-less? Let me know if I'm mistaken!)

~~BeginOnly~~
BeginAndEnd
ProcessOnly (Default in the case of a single script block.)
BeginAndProcess
ProcessAndEnd (Default in the case of by-position entry for two blocks.)
EndOnly
AllBlocks

* combinations that include -End without -Process might be able to utilise $input? I'm not sure if that's hooked up, or if it could be.

Subtractions

This would remove the ability to provide more than 3 script blocks to the command, a capability that I've personally never seen and didn't know about. I've also never heard anyone even really mention it was possible until recently.

I don't think this is a huge negative, really; anything you can do with the current behaviour, you can do in only 3 scriptblocks anyway.

Additions

Hopefully a good deal of speedup?

(And yes, I'm more than happy to sift through this myself and submit a PR, I just want to know if there's context I need to take into account / am missing, before I spend a good couple hours rewriting this.)

If I understand right we do binding for every input object. In the case the proposal is too narrow. I would consider improving the performance of pipelines as a whole. For example, we could do the binding once for collections of objects of the same type (that is actually a general case). This can be implemented in several ways.

Ilya (@iSazonov)

Ilya (@iSazonov) that may be an effective way to improve the overall speed of pipelines, but that kind of solution may have its own drawbacks, especially as it's not always possible to guarantee that the same type of object will always be sent down a pipeline.

It is possibly worth investigating, but I think that should go in its own issue. :)

If I understand right we do binding for every input object.

Only for InputObject, none of these parameters accept input. Binding is only done once for named/positional parameters:

PS> Trace-Command -Name ParameterBinding -PSHost { 0, 1 | ForEach-Object { $_ } }

DEBUG: ParameterBinding Information: 0 : BIND NAMED cmd line args [ForEach-Object]
DEBUG: ParameterBinding Information: 0 : BIND POSITIONAL cmd line args [ForEach-Object]
DEBUG: ParameterBinding Information: 0 :     BIND arg [ $_ ] to parameter [Process]
DEBUG: ParameterBinding Information: 0 :         Binding collection parameter Process: argument type [ScriptBlock], parameter type
[System.Management.Automation.ScriptBlock[]], collection type Array, element type [System.Management.Automation.ScriptBlock], no
coerceElementType
DEBUG: ParameterBinding Information: 0 :         Creating array with element type [System.Management.Automation.ScriptBlock] and 1 elements
DEBUG: ParameterBinding Information: 0 :         Argument type ScriptBlock is not IList, treating this as scalar
DEBUG: ParameterBinding Information: 0 :         Adding scalar element of type ScriptBlock to array position 0
DEBUG: ParameterBinding Information: 0 :         BIND arg [System.Management.Automation.ScriptBlock[]] to param [Process] SUCCESSFUL
DEBUG: ParameterBinding Information: 0 : MANDATORY PARAMETER CHECK on cmdlet [ForEach-Object]
DEBUG: ParameterBinding Information: 0 : CALLING BeginProcessing
DEBUG: ParameterBinding Information: 0 : BIND PIPELINE object to parameters: [ForEach-Object]
DEBUG: ParameterBinding Information: 0 :     PIPELINE object TYPE = [System.Int32]
DEBUG: ParameterBinding Information: 0 :     RESTORING pipeline parameter's original values
DEBUG: ParameterBinding Information: 0 :     Parameter [InputObject] PIPELINE INPUT ValueFromPipeline NO COERCION
DEBUG: ParameterBinding Information: 0 :     BIND arg [0] to parameter [InputObject]
DEBUG: ParameterBinding Information: 0 :         BIND arg [0] to param [InputObject] SUCCESSFUL
DEBUG: ParameterBinding Information: 0 : MANDATORY PARAMETER CHECK on cmdlet [ForEach-Object]
DEBUG: ParameterBinding Information: 0 : BIND PIPELINE object to parameters: [ForEach-Object]
DEBUG: ParameterBinding Information: 0 :     PIPELINE object TYPE = [System.Int32]
DEBUG: ParameterBinding Information: 0 :     RESTORING pipeline parameter's original values
DEBUG: ParameterBinding Information: 0 :     Parameter [InputObject] PIPELINE INPUT ValueFromPipeline NO COERCION
DEBUG: ParameterBinding Information: 0 :     BIND arg [1] to parameter [InputObject]
DEBUG: ParameterBinding Information: 0 :         BIND arg [1] to param [InputObject] SUCCESSFUL
DEBUG: ParameterBinding Information: 0 : MANDATORY PARAMETER CHECK on cmdlet [ForEach-Object]
DEBUG: ParameterBinding Information: 0 : CALLING EndProcessing

I fully agree that the current syntax is confusing, and that the utility of multiple -Process blocks is questionable.

Can't speak to how much performance would improve due to such a cleanup, but ForEach-Object could definitely use a speed boost; related in that respect: #7700

Oh yeah, I only didn't mention that in this issue because I remembered there was another issue for that one.

The logic for member resolution in ForEach-Object is really in need of some love there. I skimmed through that, and about 1/3rd of it is giant block comments, and the rest of it (which is at least three to four times as long as the scriptblock-handling code) could really use a rewrite.

Probably I'd look to have those done in separate PRs, though. 😄

Rain Sallow (/u/ta11ow) (@vexx32)

Thanks, Rain Sallow (/u/ta11ow) (@vexx32) - can you please post these insights also at #7700?

But the point of ForEach-Object is syntax and flexibility, not speed.

It doesn't really do anything, except invoke code with pipeline inputs, so while you're obviously right that you can do almost all of the things it can do with less parameters (or even without a cmdlet at all, using the call & or dot . operator) ... the syntax for that is extremely nasty. You might want to mug someone with it...

Here's a trivial example you cannot do with a ForEach-Object that only takes three blocks:

$userProvidedScript = { if($_ -eq 5) { Write-Error "TWO" -ea silentlycontinue } }
$validationScript = { if (!$?) { Write-Host "The user provided script did not work with ${_}" } }

1..10 | ForEach-Object $null,
$UserProvidedScript, 
$validationScript, 
$null

Automatic variables are (re)set between each scriptblock, so $PSItem and the other automatic variables like $? are (re)set, but otherwise, all the blocks share scope. You can set variables in one, read them in another, and always trust that $_ or $input or $PSItem will be what came in from the pipeline...

Anyway. My point is: you're talking about breaking backward compatibility (of a feature which granted, may be rarely used) on the hypothetical that you can make it faster.

Even if you were saying that you've made it 2x faster, and this is the down-side, I think I'd still be saying: maybe that should be a new command - it's not worth breaking this one (which has been like this for 12 years) to make it slightly faster, when there are so many ways to write things that are much, much faster.

How about, instead, we make the (exponentially faster, and less complex) & call operator and the dot-source operator take three scriptblocks instead of just one as an alternative syntax (i.e. & {} {} {} could be the same as &{ begin{} process{} end{} })

Heck, how about we just make & be a filter instead of a function, where the default block is the process block. Problem solved, at 5-10x speed

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

This issue has been marked as "No Activity" as there has been no activity for 6 months. It has been closed for housekeeping purposes.

Navigation Menu

Repository navigation

ForEach-Object: Wat #9408

Proposal

Subtractions

Additions

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	[Parameter(Mandatory = true, Position = 0, ParameterSetName = "ScriptBlockSet")]
	[AllowNull]
	[AllowEmptyCollection]
	public ScriptBlock[] Process
	{
	set
	{
	if (value == null)
	_scripts.Add(null);
	else
	_scripts.AddRange(value);
	}

	get
	{
	return null;
	}
	}

	[SuppressMessage("Microsoft.Performance", "CA1819:PropertiesShouldNotReturnArrays", Justification = "Cmdlets use arrays for parameters.")]
	[Parameter(ParameterSetName = "ScriptBlockSet", ValueFromRemainingArguments = true)]
	[AllowNull]
	[AllowEmptyCollection]
	public ScriptBlock[] RemainingScripts
	{
	set
	{
	if (value == null)
	_scripts.Add(null);
	else
	_scripts.AddRange(value);
	}

	get { return null; }
	}

ForEach-Object: Wat #9408

Description

Proposal

Subtractions

Additions

Activity

iSazonov commented on Apr 19, 2019

vexx32 commented on Apr 19, 2019

SeeminglyScience commented on Apr 19, 2019

mklement0 commented on Apr 19, 2019

vexx32 commented on Apr 19, 2019

mklement0 commented on Apr 20, 2019

Jaykul commented on Apr 22, 2019

microsoft-github-policy-service commented on Nov 16, 2023

microsoft-github-policy-service commented on Nov 16, 2023

microsoft-github-policy-service commented on Nov 16, 2023

microsoft-github-policy-service commented on Nov 23, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions