Skip to content
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

policy: add startup benchmark and make SRI lazier #29527

Closed
wants to merge 1 commit into from

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Sep 11, 2019

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

The lazy SRI has most significant impact on code bases not loading all possible resources (such as CLIs loading only code for specific features used). It could be made lazier to not fully parse the SRI string, but that would be complicated if different bodies are found for a resource.

This code does remove some niceties for allowing colliding resource locations (such as ./foo and ./bar/../foo) being able to have differing orderings of their SRI as long as they match and error messages, but that seems fine as collisions are an edge case. With this change, the SRI strings must be equal rather than trying to compare them.

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Sep 14, 2019

Aside: Should we eventually incorporate as many of the WPT tests for SRI (https://github.com/web-platform-tests/wpt/tree/master/subresource-integrity) as we can, the way we do for other things in test/wpt? @joyeecheung

(Also, this could use some reviews.)

@Trott Trott added the review wanted PRs that need reviews. label Sep 14, 2019
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member

@Trott Although the concepts are related, I don't think we can incorporate the WPT in their current format here, since our policy feature is not based on HTTP and Origin, and our integrity information does not come from HTML tags but a local file. For that we'll need a new more unified implementation of SRI or change how the policies work significantly.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if the tests are happy. Would be nicer if there are more comments about the shape of data being juggled around here.

lib/internal/policy/sri.js Outdated Show resolved Hide resolved
@bmeck
Copy link
Member Author

bmeck commented Sep 17, 2019

@joyeecheung would adding JSDoc be enough for explaining the data types being passed around?

@joyeecheung
Copy link
Member

joyeecheung commented Sep 17, 2019

@bmeck I think an example is already enough (and easier to digest than JSDoc). JSDoc helps when you are using an IDE but doesn't do much during code reviews.

@bmeck
Copy link
Member Author

bmeck commented Sep 17, 2019

@joyeecheung the docs have examples of the data being passed around. I'm not sure I understand the request, it seems somewhere between documenting the usage and documenting the types but I'm unsure what exactly. Could you give some sort of example and if it should be in the docs or a comment in the code?

@joyeecheung
Copy link
Member

joyeecheung commented Sep 17, 2019

@bmeck Copying the examples from the doc into the code would also work, the request was about documenting the shape of the data so it takes less effort to remember what the data look like, which can help this part of the code base getting reviewed - this is just refactoring of very abstract data crunching code i.e. you don't really need to know about the feature and even a JS programmer who are new to Node.js should be able to review this once it's explained in the comments, IMO, but it is not currently written as so.

@bmeck
Copy link
Member Author

bmeck commented Sep 17, 2019

@joyeecheung I attempted to address the nits

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM, thanks

@BridgeAR
Copy link
Member

This needs a rebase.

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@jasnell
Copy link
Member

jasnell commented Jun 19, 2020

Ping @bmeck

@bmeck
Copy link
Member Author

bmeck commented Jun 25, 2020

@jasnell will try to rebase today, I'm still hoping we get a 2nd reviewer though. Policies were quite slow on startup but easier to understand code wise before this change.

@bmeck
Copy link
Member Author

bmeck commented Jul 3, 2020

Rebased, benchmark shows a better improvement than previously

$ cat ./out.log | Rscript benchmark/compare.R
                                 confidence improvement accuracy (*)   (**)   (***)
 policy/policy-startup.js n=1024        ***     81.52 %       ±7.20% ±9.61% ±12.58%

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM

@nodejs-github-bot
Copy link
Collaborator

@bmeck
Copy link
Member Author

bmeck commented Jul 14, 2020

rebased due to error in 3 way merge while trying the normal git node land workflow, landing once CI shows green again.

@nodejs-github-bot
Copy link
Collaborator

@bmeck
Copy link
Member Author

bmeck commented Jul 14, 2020

Landed in 66810a0

@bmeck bmeck closed this Jul 14, 2020
bmeck pushed a commit that referenced this pull request Jul 14, 2020
PR-URL: #29527
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 15, 2020
PR-URL: #29527
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 15, 2020
MylesBorins pushed a commit that referenced this pull request Jul 16, 2020
PR-URL: #29527
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
cjihrig pushed a commit that referenced this pull request Jul 23, 2020
PR-URL: #29527
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
PR-URL: #29527
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
PR-URL: #29527
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review wanted PRs that need reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants