hoist boundary check code to the out of loop #10

Merged
merged 2 commits into from Dec 3, 2014

Conversation

Projects
None yet
2 participants
@imasahiro
Contributor

imasahiro commented Dec 2, 2014

Current implementation of sprintf(..., "%x",...) always execute boundary check.
(e.g. https://github.com/h2o/qrintf/blob/master/share/qrintf/qrintf.h#L1321)

This pull request hoists this check to the out of loop to simplify main code of hexadecimal to string conversion.

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Dec 2, 2014

Member

Thank you for the PR.

I agree that boundary check should not be performed for every byte, I am basically fine to merge the changes. Before that, would you mind making following changes so that the diff would be minimal?

  • there is no need to introduce *p, IMO using ctx.str[ctx.off++] should be fine in terms of performance
    • the reasons why I believe so is:
    • both ctx.str and ctx.off would be stored on register since they are always passed-by-value
    • the expression can be compiled into movb %r1, (%r2,%r3) or something alike
    • ctx needs to be returned, and using the value to store both intermediate and final results will save the number of registers used
  • do not introduce wlen as well - if possible, please update len for the purpose in order to save the registers

It would be great if you could make such changes so that the diff of gets limited to the topic of the branch (i.e. only change the _qrintf_chk_?x functions).

I would appreciate it if you could either open a separate PR or a separate commit within this PR to change other things in case you are convinced that my understanding described in the above listing is wrong.

Member

kazuho commented Dec 2, 2014

Thank you for the PR.

I agree that boundary check should not be performed for every byte, I am basically fine to merge the changes. Before that, would you mind making following changes so that the diff would be minimal?

  • there is no need to introduce *p, IMO using ctx.str[ctx.off++] should be fine in terms of performance
    • the reasons why I believe so is:
    • both ctx.str and ctx.off would be stored on register since they are always passed-by-value
    • the expression can be compiled into movb %r1, (%r2,%r3) or something alike
    • ctx needs to be returned, and using the value to store both intermediate and final results will save the number of registers used
  • do not introduce wlen as well - if possible, please update len for the purpose in order to save the registers

It would be great if you could make such changes so that the diff of gets limited to the topic of the branch (i.e. only change the _qrintf_chk_?x functions).

I would appreciate it if you could either open a separate PR or a separate commit within this PR to change other things in case you are convinced that my understanding described in the above listing is wrong.

@imasahiro

This comment has been minimized.

Show comment
Hide comment
@imasahiro

imasahiro Dec 2, 2014

Contributor

@kazuho
Thanks for the feedback. I updated the branch with the suggested changes:

  • remove *p and use ctx.str[off++] in _qrintf_chk_?x, use ctx.str[ctx.off++] in _qrintf_nck_?x.
  • remove wlen
Contributor

imasahiro commented Dec 2, 2014

@kazuho
Thanks for the feedback. I updated the branch with the suggested changes:

  • remove *p and use ctx.str[off++] in _qrintf_chk_?x, use ctx.str[ctx.off++] in _qrintf_nck_?x.
  • remove wlen
@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Dec 2, 2014

Can't the block be rewritten as:

if (ctx.off + len > ctx.size) {
    size_t n = ctx.off + len - ctx.size;
    len -= n;
    v >>= n * 4;
}

Can't the block be rewritten as:

if (ctx.off + len > ctx.size) {
    size_t n = ctx.off + len - ctx.size;
    len -= n;
    v >>= n * 4;
}
@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Dec 2, 2014

IMO there's no need to define off

IMO there's no need to define off

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Dec 2, 2014

With the changes above, we can use line 127 for 'chk' mode as well.

With the changes above, we can use line 127 for 'chk' mode as well.

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Dec 2, 2014

Member

Thanks for the changes. I think we are almost complete. Please consider the nit-picking comments on the commit.

Member

kazuho commented Dec 2, 2014

Thanks for the changes. I think we are almost complete. Please consider the nit-picking comments on the commit.

@imasahiro

This comment has been minimized.

Show comment
Hide comment
@imasahiro

imasahiro Dec 2, 2014

Contributor

Thank you for your comment.

But we cannot modify the code as above because it breaks a compatibility with snprintf.
Acording to man page of printf(http://linux.die.net/man/3/sprintf), when the
buffer does not have enough space to write, the return value of the snprintf
is the number of characters which would have been written to the final string
if enough space has been available.

Return Value
...
If the output was truncated due to this limit then the return value
is the number of characters (excluding the terminating null byte)
which would have been written to the final string if enough space
had been available.

Contributor

imasahiro commented Dec 2, 2014

Thank you for your comment.

But we cannot modify the code as above because it breaks a compatibility with snprintf.
Acording to man page of printf(http://linux.die.net/man/3/sprintf), when the
buffer does not have enough space to write, the return value of the snprintf
is the number of characters which would have been written to the final string
if enough space has been available.

Return Value
...
If the output was truncated due to this limit then the return value
is the number of characters (excluding the terminating null byte)
which would have been written to the final string if enough space
had been available.

@imasahiro

This comment has been minimized.

Show comment
Hide comment
@imasahiro

imasahiro Dec 2, 2014

Contributor

I tried to reconsider about above comments and I updated the branch.

  • Remove local variable off
  • Introduce new local variable rest
  • Both qrintf_chk?x and qrintf_nck?x now use ctx.str[ctx.off++]
Contributor

imasahiro commented Dec 2, 2014

I tried to reconsider about above comments and I updated the branch.

  • Remove local variable off
  • Introduce new local variable rest
  • Both qrintf_chk?x and qrintf_nck?x now use ctx.str[ctx.off++]
@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Dec 3, 2014

Member

Hmm. Let me see. Thought that it was possible to not add any local vars...

Member

kazuho commented Dec 3, 2014

Hmm. Let me see. Thought that it was possible to not add any local vars...

@kazuho kazuho merged commit 008a174 into h2o:master Dec 3, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

kazuho added a commit that referenced this pull request Dec 3, 2014

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Dec 3, 2014

Member

Sorry for the fuss. The proposed code seems optimal. Merged to master with little tweaks.

Member

kazuho commented Dec 3, 2014

Sorry for the fuss. The proposed code seems optimal. Merged to master with little tweaks.

@imasahiro imasahiro deleted the imasahiro:hoist_check branch Dec 3, 2014

@imasahiro

This comment has been minimized.

Show comment
Hide comment
@imasahiro

imasahiro Dec 3, 2014

Contributor

@kazuho
Thank you very much for your review and final merge.

Contributor

imasahiro commented Dec 3, 2014

@kazuho
Thank you very much for your review and final merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment