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.

Sign up for GitHub

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

Jump to bottom

url: always show password for URL instances #12420

Closed
wants to merge 1 commit into from
Closed

url: always show password for URL instances #12420

wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Apr 15, 2017 •
edited
Loading

This matches browser behavior.

CI: https://ci.nodejs.org/job/node-test-pull-request/7413/

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
  • url

@mscdex mscdex added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Apr 15, 2017
@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Apr 15, 2017
lpinca
lpinca reviewed Apr 15, 2017
View reviewed changes
lib/internal/url.js Outdated
const innerOpts = Object.assign({}, ctx);
if (recurseTimes !== null) {
innerOpts.depth = recurseTimes - 1;
var separator = ', ';
Copy link
Member

Choose a reason for hiding this comment

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

Is performance the reason for changing const to var?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, permanent deopt.

Copy link
Member

Choose a reason for hiding this comment

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

Didn't read commit message, sorry.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a V8 bug for this? cc @nodejs/v8

Copy link
Contributor Author

@mscdex mscdex Apr 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@joyeecheung I don't think it's a 'bug' per se, just something that isn't optimized yet in the version of V8 in master.

Copy link
Member

Choose a reason for hiding this comment

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

I think we might want to add a comment here in case someone feels like replacing those with const again.

Copy link
Contributor Author

@mscdex mscdex Apr 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure it needs a comment. Many optimizations are made without comments describing why they exist, including prior permanent deopt fixes. Also, the hope is that V8 will (or already has) fix the deopt (perhaps with ignition + turbofan... I don't recall which pipeline the function was being optimized under).

Ideally such permanent deopts would be checked when pushing such code in the first place. Perhaps having a Makefile target that runs the parallel tests with --trace-opt --trace-deopt and greps for /aborted|disabled/ would be helpful in achieving this in an automated way.

Copy link
Member

@benjamingr benjamingr Apr 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Ideally, I agree - but since it was missed the first time around in mscdex@6b6123c#diff-7fa9474fccb31c3ccef5d728fbd5248bR865 and I don't think performance regressions are run against every small refactoring I think a comment would still be useful in this particular case.

If we can figure how to do it in an automated way that would be great, I don't think contributors are not very familiar with the basic performance tooling like --trace-deopt let alone can understand their output very easily.

Copy link
Contributor Author

@mscdex mscdex Apr 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@benjamingr It's not just this one line though. Many times all const/let in the containing function has to be changed before the perm deopt goes away.

As far as checking for permanent deopts, the method I described works well enough:

node --trace-opt --trace-deopt foo.js | grep -iP 'abort|disabled'

If anything shows up, then there is a perm deopt somewhere. The name of the function or variable is included in the output FWIW.

Copy link
Member

Choose a reason for hiding this comment

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

Then I'm +1 on adding it to the makefile, worst case someone will not understand it and open an issue and we can discuss whether or not it's confusing then, good case - people understand it right off the bat.

TimothyGu
TimothyGu approved these changes Apr 15, 2017
View reviewed changes
lib/internal/url.js Outdated
@@ -874,32 +871,32 @@ class URLSearchParams {
this[context] = null;
}

[util.inspect.custom](recurseTimes, ctx) {
[util.inspect.custom](depth, opts) {
Copy link
Member

Choose a reason for hiding this comment

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

I prefer the recurseTimes/ctx wording since that's what's used in util.js.

@jasnell
Copy link
Member

jasnell commented Apr 15, 2017

I'm good with this but it may be worthwhile putting the const to var changes in their own commit that can be easily reverted once the optimization issue is resolved in master

@mscdex mscdex mentioned this pull request Apr 17, 2017
Merged
3 tasks
@mscdex
Copy link
Contributor Author

mscdex commented Apr 17, 2017

I might just fold the deopt fixes into #12456 instead...

@mscdex mscdex changed the title url: various changes url: always show password for URL instances Apr 24, 2017
@mscdex
Copy link
Contributor Author

mscdex commented Apr 24, 2017 •
edited
Loading

Ok, I've removed the deopt change and I guess I'll just forget the variable name changes.... I'll let someone else submit a PR if they want to use consistent names. So now it's just the password change.

@mscdex
Copy link
Contributor Author

mscdex commented Apr 24, 2017

CI again: https://ci.nodejs.org/job/node-test-pull-request/7630/

TimothyGu
TimothyGu approved these changes Apr 24, 2017
View reviewed changes
Copy link
Member

@TimothyGu TimothyGu 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.

@TimothyGu
Copy link
Member

This needs a rebase.

@mscdex
url: always show password for URL instances
bd7c5c9 
This matches browser behavior.
@mscdex
Copy link
Contributor Author

mscdex commented Apr 25, 2017 •
edited
Loading

@TimothyGu Done.

CI: https://ci.nodejs.org/job/node-test-pull-request/7681/

jasnell pushed a commit that referenced this pull request Apr 26, 2017
@mscdex @jasnell
url: always show password for URL instances
d7ba2a6 
This matches browser behavior.

PR-URL: #12420
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@jasnell
Copy link
Member

jasnell commented Apr 26, 2017

Landed in d7ba2a6

@jasnell jasnell closed this Apr 26, 2017
@mscdex mscdex deleted the url-whatwg-changes branch April 26, 2017 16:27
@evanlucas evanlucas mentioned this pull request May 1, 2017
Merged
evanlucas pushed a commit that referenced this pull request May 1, 2017
@mscdex @evanlucas
url: always show password for URL instances
a2d3161 
This matches browser behavior.

PR-URL: #12420
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
evanlucas pushed a commit that referenced this pull request May 2, 2017
@mscdex @evanlucas
url: always show password for URL instances
18aec67 
This matches browser behavior.

PR-URL: #12420
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
evanlucas pushed a commit that referenced this pull request May 2, 2017
@mscdex @evanlucas
url: always show password for URL instances
60daaae 
This matches browser behavior.

PR-URL: #12420
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca left review comments

@benjamingr benjamingr benjamingr approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@watilde watilde watilde approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@mscdex @jasnell @TimothyGu @benjamingr @lpinca @watilde @joyeecheung @nodejs-github-bot