Bug 117497 – Improve algorithm of removing user and pass, by seperating new function

• Status: UNCONFIRMED
• Component: Platform

[Meeyoung Kim]

When removing user and pass from url, setUser() and setPass() are called, together.

[Meeyoung Kim]

Created attachment 204298 [details]
Improve algorithm of removing user and pass, by seperating removeUserPass()

Reduce the function calls when removing the user and pass from url.

[Alexey Proskuryakov]

I'm confused by the status of this bug. It's marked RESOLVED/FIXED - when was this patch landed?

[Meeyoung Kim]

I am waiting to review of the patch. Which state is right? Unconfirmed? 
(In reply to comment #2)
> I'm confused by the status of this bug. It's marked RESOLVED/FIXED - when was this patch landed?

[Alexey Proskuryakov]

Unconfirmed, new or reopened would all be fine.

[Darin Adler]

Comment on attachment 204298 [details]
Improve algorithm of removing user and pass, by seperating removeUserPass()

View in context: https://bugs.webkit.org/attachment.cgi?id=204298&action=review

I’m not sure about this patch. Improving performance of a hot code path seems well worth it. If this code path is hot. The changes to setUser and setPass seem to be unmotivated; they remove the optimization for the empty string, without increasing code clarity significantly. I’d just leave those functions alone.

Since we are proposing this patch as a performance optimization, I would love to see data about whether a speedup is measurable. I’m not sure it’s worth making changes here that add a new function and bit more code unless there is a real benefit.

I’m going to say review- because the benefit seems so marginal. But I could be convinced to change my mind. It is more elegant to remove both of these at once, but I’d prefer not to expand the number of functions in the KURL class except to do something truly worthwhile. There are only two call sites for this!

> Source/WebCore/platform/KURL.cpp:776
> +    parse(m_string.left(m_userStart) + u + m_string.substring(end));

This removes the small performance optimization we used to have, where we don’t re-parse if we are not making a change. Please don’t remove that in a patch that is supposed to be about optimization. Perhaps you rationale is that you think the only call sites that needed this optimization were the same ones that are calling the new function. But still, I don’t think this change is a significant improvement.

> Source/WebCore/platform/KURL.cpp:800
> +    parse(m_string.left(m_userEnd) + p + m_string.substring(end));

Same issue here.

> Source/WebCore/platform/KURL.cpp:802
> +}
> +void KURL::removeUserPass()

For a new function, I suggest we use naming without abbreviation: removeUserAndPassword.

Also, need a blank line here.

> Source/WebCore/platform/network/ResourceRequestBase.cpp:137
>      if (m_url.user().isEmpty() && m_url.pass().isEmpty())
>          return;

Since the idea is to improve performance, I suggest we write this in a way that avoids having to build a string for the user and another string for password just to check if they are present. The way to do that would be to add a boolean return value from removeUserAndPassword and return true if anything was removed.

[Meeyoung Kim]

Created attachment 204876 [details]
Improve algorithm of removing user and pass, by seperating new function.Building the User and passward is avoided By using the new function.

[Darin Adler]

Comment on attachment 204876 [details]
Improve algorithm of removing user and pass, by seperating new function.Building the User and passward is avoided By using the new function.

View in context: https://bugs.webkit.org/attachment.cgi?id=204876&action=review

I still don’t see any data about whether this is a measurable speedup or not.

> Source/WebCore/platform/KURL.cpp:819
> +    if (m_userStart != end) {
> +        parse(m_string.left(m_userStart) + m_string.substring(end));
> +        return true;
> +    }
> +    return false;

This would read better as an early return:

    if (m_userStart == end)
        return false;

[Meeyoung Kim]

Created attachment 205476 [details]
code refactoring as  mentioned of reviewer


I tried to find a way to measure the performance though, it was required to make URL requests in the testcase to see the difference and the result are very affected by many external variables such as network, resource loading etc. 

As you mentioned before, it is small performance optimization amd fixes are so trivial that I think it's not a problem appying path without measurment.

[Early Warning System Bot]

Comment on attachment 205476 [details]
code refactoring as  mentioned of reviewer

Attachment 205476 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/919198

[EFL EWS Bot]

Comment on attachment 205476 [details]
code refactoring as  mentioned of reviewer

Attachment 205476 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/978766

[Early Warning System Bot]

Comment on attachment 205476 [details]
code refactoring as  mentioned of reviewer

Attachment 205476 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/850948

[EFL EWS Bot]

Comment on attachment 205476 [details]
code refactoring as  mentioned of reviewer

Attachment 205476 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/916648

[kov's GTK+ EWS bot]

Comment on attachment 205476 [details]
code refactoring as  mentioned of reviewer

Attachment 205476 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/880727

[Build Bot]

Comment on attachment 205476 [details]
code refactoring as  mentioned of reviewer

Attachment 205476 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/919199

[Build Bot]

Comment on attachment 205476 [details]
code refactoring as  mentioned of reviewer

Attachment 205476 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/849477

[Build Bot]

Comment on attachment 205476 [details]
code refactoring as  mentioned of reviewer

Attachment 205476 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/912875

[Meeyoung Kim]

Created attachment 205554 [details]
re-upload the patch after fix the error

Fix error before patch

[Darin Adler]

Comment on attachment 205554 [details]
re-upload the patch after fix the error

View in context: https://bugs.webkit.org/attachment.cgi?id=205554&action=review

Definitely don’t want to land this with the extra m_userStart == end check in it. But also, I want some reassurance that we have test coverage for the code that is looking for a "@" character. And a comment explaining why it's there.

> Source/WebCore/platform/KURL.cpp:808
>  }
> +bool KURL::removeUserAndPassword()

Need a blank line between functions.

> Source/WebCore/platform/KURL.cpp:818
> +    if (m_userStart == end)
> +        return false;
> +
> +    if (m_userStart != end && end != m_hostEnd && m_string[end] == '@')
> +        end += 1;

This checks m_userStart == end twice in a row.

The end += 1 code is too confusing without a why comment. It seems like we copied it from setPass, but removed the comment and explanation.

I also have no idea why the end += 1 is needed. I doubt that there can be a "@" character *after* m_passwordEnd. Is that really something that happens in our parsing? Is there a test case that covers this?

> Source/WebCore/platform/KURL.cpp:822
> +}
>  void KURL::setFragmentIdentifier(const String& s)

Need a blank line between functions.

[Csaba Osztrogonác]

Comment on attachment 204876 [details]
Improve algorithm of removing user and pass, by seperating new function.Building the User and passward is avoided By using the new function.

Cleared Darin Adler's review+ from obsolete attachment 204876 [details] so that this bug does not appear in http://webkit.org/pending-commit.