Skip to content

Conversation

@masaori335
Copy link
Contributor

  • Avoid memcpy & parse for setting URL
  • Avoid unnecessary MIMEField search by calling field_delete(const char *name, int name_length)

@masaori335 masaori335 added this to the 10.0.0 milestone Mar 23, 2020
@masaori335 masaori335 self-assigned this Mar 23, 2020
@masaori335 masaori335 linked an issue Mar 23, 2020 that may be closed by this pull request
12 tasks
const char *scheme = field->value_get(&scheme_len);

int scheme_wks_idx = hdrtoken_tokenize(scheme, scheme_len);
url_scheme_set(headers->m_heap, headers->m_http->u.req.m_url_impl, scheme, scheme_wks_idx, scheme_len, true);
Copy link
Member

Choose a reason for hiding this comment

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

Does this work even if the scheme is not wks? Or can we ignore the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC, url_scheme_set() copy scheme when bool copy_string flag (the last arg) is true even if scheme_wks_idx is -1.

maskit
maskit previously approved these changes Mar 25, 2020
@maskit maskit dismissed their stale review March 25, 2020 00:35

autest is failed

@maskit
Copy link
Member

maskit commented Mar 25, 2020

[approve ci autest]

@masaori335
Copy link
Contributor Author

Running Test http2:......F. Failed

I'll dig this failure.

- Avoid memcpy & parse for setting URL
- Avoid unnecessary MIMEField search
@masaori335 masaori335 force-pushed the h2-perf-hdr-h2-to-h1 branch from 5845135 to 37c2758 Compare March 25, 2020 01:52
@masaori335
Copy link
Contributor Author

The failure comes from treating the path. url_print() add /, so / needs to be removed.
Prior to this PR, url_parse() removed it.

DEBUG: <URL.cc:1607 (url_describe)> (http)     PATH: "huge_resp_hdrs", PATH_LEN: 14,

vs

DEBUG: <URL.cc:1607 (url_describe)> (http)     PATH: "/huge_resp_hdrs", PATH_LEN: 15,

const char *path = field->value_get(&path_len);

// cut first '/' if there, because `url_print()` add '/' before printing path
if (path_len >= 1 && path[0] == '/') {
Copy link
Member

Choose a reason for hiding this comment

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

Is this the same as what url_parse does? What if there are multiple slashes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had the same question for multiple slashes and tried. If my experiment is correct, the only first / is removed.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I'm fine fine with this as long as the behavior is the same.

@masaori335 masaori335 merged commit 5810f13 into apache:master Apr 3, 2020
@masaori335 masaori335 mentioned this pull request Apr 14, 2020
12 tasks
@masaori335 masaori335 removed a link to an issue Jul 2, 2020
12 tasks
@zwoop zwoop modified the milestones: 10.0.0, 9.1.0 Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants