policy: add startup benchmark and make SRI lazier #29527
Conversation
|
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 (Also, this could use some reviews.) |
|
@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. |
|
@joyeecheung would adding JSDoc be enough for explaining the data types being passed around? |
|
@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. |
|
@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? |
|
@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. |
|
@joyeecheung I attempted to address the nits |
|
This needs a rebase. |
BridgeAR
force-pushed
the
master
branch
2 times, most recently
from
8ae28ff
to
2935f72
Compare
|
Ping @bmeck |
|
@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. |
|
Rebased, benchmark shows a better improvement than previously |
|
rebased due to error in 3 way merge while trying the normal |
|
Landed in 66810a0 |
PR-URL: #29527 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #29527 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #29527 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #29527 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #29527 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #29527 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesThe 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
./fooand./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.