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

Optimize process kill by pid #16668

Merged
merged 1 commit into from Mar 3, 2018

Conversation

Projects
None yet
4 participants
@Connormiha
Contributor

Connormiha commented Feb 2, 2018

Description of the Change

Minor refactoring code for killing process.
Used 1 for loop instead of 4 for loops

Benefits

Improve performance

.map((pid) => parseInt(pid))
.filter((pid) => pid !== parentPid && pid > 0 && pid < Infinity)
for (let pid of output.split(/\s+/)) {
if (!isFinite(pid)) continue

This comment has been minimized.

@Ingramz

Ingramz Feb 3, 2018

Contributor

Should be redundant because parseInt only returns an integer value or NaN, both cases are covered at the check below.

E: Also returns Infinity for a long enough string, but that is also covered below.

This comment has been minimized.

@Connormiha

Connormiha Feb 3, 2018

Contributor

This check allow to aviod parse digits + text. For example.

let num = '12345aaaaa';
parseInt(num); // 12345
isFinite(num); // false

In this case we should parse only clear number, without text tail.
Before was regexp /^\d+$/

This comment has been minimized.

@Ingramz

Ingramz Feb 3, 2018

Contributor

Fair enough. What about floats?

let num = '1.2345';
parseInt(num); // 1
isFinite(num); // true

This comment has been minimized.

@Connormiha

Connormiha Feb 3, 2018

Contributor

Ok. What do you think about use Number.isSafeInteger instead of ifFinite?

// if (!isFinite(pid)) continue
pid = +pid
if (!Number.isSafeInteger(pid)) continue

Number.isSafeInteger(+'1234'); // false
Number.isSafeInteger(+'1.1'); // false
Number.isSafeInteger(+'123a'); // false
Number.isSafeInteger(+'123a'); // false

As bonus we can get rid of parseInt and check for Infinity

Only a very incredible case pass

Number.isSafeInteger(+'1.'); // true
Number.isSafeInteger(+'1.0'); // true

I think it is not neccessery, but we can use regExp to be sure 100%

So the final code is:

for (let pid of output.split(/\s+/)) {
        if (!/^\d{1,8}$/.test(pid)) continue
        pid = parseInt(pid, 10);

        if (!pid || pid === parentPid) continue

        try {
          process.kill(pid)
        } catch (error) {}
}

This comment has been minimized.

@Ingramz

Ingramz Feb 3, 2018

Contributor

The final code looks a lot more reasonable. Just increase the 8 digits in regex to something that can contain all unsigned 32bit integers, maybe 10.

@atom atom deleted a comment from Connormiha Feb 3, 2018

@Ingramz

Ingramz approved these changes Feb 3, 2018

.filter((pid) => pid !== parentPid && pid > 0 && pid < Infinity)
for (let pid of output.split(/\s+/)) {
if (!/^\d{1,10}$/.test(pid)) continue
pid = parseInt(pid, 10)

This comment has been minimized.

@YurySolovyov

YurySolovyov Feb 21, 2018

I'd use Number.parseInt for clarity

This comment has been minimized.

@Connormiha

Connormiha Feb 21, 2018

Contributor

In whole project used global parseInt. I think migration to Number.parseInt should be in special PR.

@maxbrunsfeld maxbrunsfeld merged commit 40c608d into atom:master Mar 3, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Mar 3, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment