Store NVDA process ID in globalVars and use it consistently in the entire code base.

[lukaszgo1]

Link to issue number:

None, noticed when working on #13366

Summary of the issue:

Process ID of NVDA is retrieved and used inconsistently in the code base:

• In appModuleHandler we're storing it as a module level variable when initializing, however it is also pointlessly reinitialized when user re-initializes appModules - this makes no sense as process ID is constant during NVDA's life time.
• Most modules retrieve it by calling os.getpid
• In some cases win32 function GetCurrentProcessId is used directly.

Description of how this pull request fixes the issue:

NVDA process ID is stored in globalVars when NVDA starts - all usages of os.getpid and GetCurrentProcessId now use the new variable. Since appModuleHandler.NVDAProcessID is gone appropriate backward compatibility solution for add-ons has been implemented.

Testing strategy:

• Checked that proper appModule is loaded for NVDA process
• Checked that UIA is not used for GUI controls in NVDA
• Checked that script for announcing new line is not bound in NVDA's own edit fields and that it is bound in other applications
• Checked that accessing appModuleHandler.NVDAProcessID logs a deprecation warning but the process ID is returned
• Checked that accessing nonexistent attribute on appModuleHandler raises proper exception

Known issues with pull request:

None known

Change log entries:

For Developers

• appModuleHandler.NVDAProcessID is deprecated - use globalVars.appPid instead.

Code Review Checklist:

• Pull Request description:
  • description is up to date
  • change log entries
• Testing:
  • Unit tests
  • System (end to end) tests
  • Manual testing
• API is compatible with existing add-ons.
• Documentation:
  • User Documentation
  • Developer / Technical Documentation
  • Context sensitive help for GUI changes
• UX of all users considered:
  • Speech
  • Braille
  • Low Vision
  • Different web browsers
  • Localization in other languages / culture than English

[AppVeyorBot]

• PASS: Unit tests.
• PASS: Translation comments check.
• FAIL: System tests. See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/d6b7ukj3heviv8hk/artifacts/output/nvda_snapshot_pr13646-25314,c22b531c.exe
• PASS: Lint check.

See test results for failed build of commit c22b531ceb

[mzanm]

I wonder, why NVDA can't use UIA in it's controls?

[michaelDCurran]

Using UIA from your own process still seems to cause lockups/freezes unfortunately. This has been true since UIA was introduced in Windows 7.

[jcsteh]

Some (probably academic) thoughts:

Do we have any reason to believe that os.getpid() has a performance cost? If not, I wonder whether it'd make sense to switch everything to use that for the sake of being more standard. I think we might have assumed it did once upon a time (the logic being that function calls must be more expensive), but I doubt that was ever actually measured.

class GV:
 pid = os.getpid()

def r():
 t = time.time()
 for i in range(1000000):
  p = os.getpid()
 print("os.getpid: %s" % (time.time() - t))
 t = time.time()
 for i in range(1000000):
  p = GV.pid
 print("GV.pid: %s" % (time.time() - t))

It does look like os.getpid is slightly slower:

>>> r()
os.getpid: 0.06792235374450684
GV.pid: 0.04499697685241699

So os.getpid takes 1.5x the time of a global variable... but this is probably insignificant given the number of calls we make. Still, it also costs us nothing to optimise it, so maybe it's worthwhile to have the variable. 🤷‍♂️

[lukaszgo1]

class GV:
 pid = os.getpid()

I don't think testing with a class attribute here makes sense as we never stored PID on a class - it was either stored at a module level or as a local variable inside the function. As demonstrated below this seems to make a difference (I've tested just with a module level variable though).

Based on your example:

mod_level_pid = os.getpid()
# and then inside the r function:
	t = time.time()
	for i in range(1000000):
		p = mod_level_pid
	print("mod_level_pid: %s" % (time.time() - t))

The results are:

os.getpid: 0.16000008583068848
GV.pid: 0.08000016212463379
mod_level_pid: 0.06000018119812012

The module level variable is the fastest of these tree it seems.

To have a full picture I've also measured how slow is a module attribute variable, by creating a module called gv_mod and then expanding the r function with:

	t = time.time()
	for i in range(1000000):
		p = gv_mod.pid
	print("gv_mod.pid: %s" % (time.time() - t))

The results are:

os.getpid: 0.16000008583068848
GV.pid: 0.07999992370605469
gv_mod.pid: 0.09999990463256836
mod_level_pid: 0.07000017166137695

To summarize while looking up a variable from an different module is a little bit faster than os.getpid the difference is much smaller than it was with a variable in the same module. If we agree that having a global variable in appModuleHandler is not necessary the PID stored in globalVars seems to be the the best option in terms of performance.

So os.getpid takes 1.5x the time of a global variable... but this is probably insignificant given the number of calls we make. Still, it also costs us nothing to optimise it, so maybe it's worthwhile to have the variable. man_shrugging

Optimizing this code path was not a goal of this PR indeed. Obviously having a performance measurements here is nice, but this probably is more of a style discussion at this point than anything else.

[seanbudd]

Apologies for the late review.
LGTM other than a minor typo, which I can fix when merging.

[seanbudd]

I am not including the deprecation notice in the changelog, as we do not plan to remove this code. The getattr warning should suffice for add-on developers using the deprecated variable.

[XLTechie]

A very minor point, but isn't it customary to capitalize PID entirely? It is so in Microsoft docs. Thus: globalVars.appPID Also, what is the reason this is "appPid"--I.E. why the "app" instead of just pid? In globalVars, there shouldn't be any other PID expected, should there?

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests. See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/m6trdvg8fol2u2wo/artifacts/output/nvda_snapshot_pr13646-25520,f4950136.exe

See test results for failed build of commit f49501363d

[lukaszgo1]

Also, what is the reason this is "appPid"--I.E. why the "app" instead of just pid? In globalVars, there shouldn't be any other PID expected, should there?

This follows the way in which other variables in globalVars are named i.e. appDir and appArgs. I don't have full context why this way of naming was used, but if I would have to guess it is just to avoid very common names such as dir and args standing alone. Also it is not impossible we would need to store some different PIDinglobalVars` in the future, perhaps for some helper process or other, so this prefixing does more good than harm IMHO.

[CyrilleB79]

I am not including the deprecation notice in the changelog, as we do not plan to remove this code. The getattr warning should suffice for add-on developers using the deprecated variable.

IMO, if there is the warning, there should also be the change log information. There is no point for add-on developers to discover by chance in the log that a variable is deprecated.
If a variable is deprecated, that means that it is still supported but that it is discouraged to use it in new code. If you want to discourage ad-on writers to use this variable, informing them through both the change log and a warning is better than only through the warning only.

On the opposite, if there should not be a preferred way to access the process ID, issuing a deprecation warning makes no sense.

Note: Indicating a deprecation does not require to specify a removal date for the old syntax. It just indicates that there is a preferred syntax to use.

[seanbudd]

I think either removing the warning or adding the changelog item is fine in this instance.
I'm going to lean towards adding the changelog item and keeping the warning as that's how the PR was submitted.

[lukaszgo1]

I'm going to lean towards adding the changelog item and keeping the warning as that's how the PR was submitted.

+1 to this.

[XLTechie]

I agree with @CyrilleB79. If a developer does see the warning in the log, and tries then to find it in the changelog and can't, it will probably be thought to be an error. This seems inconsistent. IMHO it should be specified both ways.