Skip to content

Conversation

iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented Apr 9, 2020

PR Summary

Fix #3322 on Windows and Linux. For MacOs we will open new tracking issue to implement later.

PR Context

PR Checklist

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Apr 9, 2020
@iSazonov iSazonov added this to the 7.1.0-preview.2 milestone Apr 9, 2020
@ghost ghost assigned adityapatwardhan Apr 9, 2020
@iSazonov iSazonov marked this pull request as ready for review April 9, 2020 18:21
@adityapatwardhan
Copy link
Member

@iSazonov can you give a before and after for a perf comparison? Call Get-CimInstance or Get-Content for every System.Diagnostics.Process object sounds expensive.

The test can be as simple as:

Measure-command { 1..100 | % { get-process } }

@iSazonov
Copy link
Collaborator Author

@adityapatwardhan It is "on demand" property - you evaluate it only if you explicitly call the property. But if you call you get a slowdown. It is not in default output so users don't see slowdown in common scenarios.
Notice, I found WMI is very fast on latest Windows versions (Windows 10 and Windows Server 2019). If someone provides feedback that it's still slow we could consider a P/Invoke.
On Linux /proc works fast.
For MacOs I do not know API. I am going to open new tracking issue for this after the PR will be merged.

@SteveL-MSFT
Copy link
Member

For both Linux and macOS, I wonder if it would be ok to run ps to cache the command lines and map it to the object?

@iSazonov
Copy link
Collaborator Author

Using /proc is simple and reliable on Linux so no need to create a workaround.
I haven't Mac and can not create even a workaround. Nonetheless I hope there is a good API for that.

@adityapatwardhan
Copy link
Member

@TravisEz13 @JamesWTruher Do you know of a macOS workaround for this?

@adityapatwardhan
Copy link
Member

@iSazonov I am not sure if we should take this PR without having a solution for all platforms. This introduces gotchas and makes cross platform scripting difficult.

@iSazonov @SteveL-MSFT - thoughts?

@iSazonov
Copy link
Collaborator Author

My plan is to open a tracking issue for MacOs. We have a lot of time before release and can investigate how enhance the feature on MacOs.

@powercode
Copy link
Collaborator

powercode commented May 8, 2020

The APIs will probably require some native coding on the mac.

We can probably use the same apis as top.

https://opensource.apple.com/source/top/top-111.20.1/libtop.c.auto.html

@ghost ghost added the Review - Needed The PR is being reviewed label May 27, 2020
@ghost
Copy link

ghost commented May 27, 2020

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Mainainer, Please provide feedback and/or mark it as Waiting on Author

@adityapatwardhan
Copy link
Member

@iSazonov Please open a tracking bug for macOS and also open a issue for documentation as per the PR template.

@ghost ghost removed the Review - Needed The PR is being reviewed label May 28, 2020
@iSazonov
Copy link
Collaborator Author

@adityapatwardhan Tracking issue #12832

As for doc issue - do we really need to document this? It seems we haven't docs for extended properties at all.
/cc @sdwheeler

@sdwheeler
Copy link
Collaborator

@iSazonov No, we don't document the extended properties. But this is worth adding to the release notes.

@adityapatwardhan adityapatwardhan merged commit c7455fd into PowerShell:master May 29, 2020
@adityapatwardhan
Copy link
Member

@iSazonov Thank you for your contribution!

@iSazonov iSazonov deleted the ps-commandline branch May 31, 2020 04:56
@ghost
Copy link

ghost commented Jun 25, 2020

🎉v7.1.0-preview.4 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend Process with CommandLine property
6 participants