uplex-varnish/libvmod-re
7
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

some minor optimizations #7

Merged
slink merged 4 commits from master into master 2019-09-13 12:23:10 +00:00
Conversation 7 Commits 4 Files changed 1 +8 -12
slink commented 2019-05-16 13:50:05 +00:00 (Migrated from code.uplex.de)
Copy link

Avoid copying if the backref equals the subject tail

Avoid copying if the backref equals the subject tail
geoff commented 2019-05-20 10:08:10 +00:00 (Migrated from code.uplex.de)
Copy link

assigned to @geoff

assigned to @geoff
geoff commented 2019-05-20 10:40:34 +00:00 (Migrated from code.uplex.de)
Copy link

We could merge the other two commits, to make the code less verbose (the compiler is probably already re-using the common subexpression).

But I don't think we need to copy or account for any terminating null byte of the subject string in order to extract backrefs, because the ovector math doesn't need it.

We could merge the other two commits, to make the code less verbose (the compiler is probably already re-using the common subexpression). But I don't think we need to copy or account for any terminating null byte of the subject string in order to extract backrefs, because the ovector math doesn't need it.
geoff commented 2019-05-20 10:49:39 +00:00 (Migrated from code.uplex.de)
Copy link

Oh, just occurred to me that you're trying to avoid the additional copy into workspace. WS_Printf() is necessary in backref() if we're copying from the middle of the string, so that the return string has a terminating null byte. But we wouldn't have to copy if a \0 were already there.

Damn you're stingy with workspace.

All right, but then match() will have to copy the string with the \0 as well. Think about that very carefully, because we'll be using another byte of workspace! And we might not need it, it's only any good for the "tail" case.

Oh, just occurred to me that you're trying to avoid the additional copy into workspace. ``WS_Printf()`` is necessary in ``backref()`` if we're copying from the middle of the string, so that the return string has a terminating null byte. But we wouldn't have to copy if a ``\0`` were already there. Damn you're stingy with workspace. All right, but then ``match()`` will have to copy the string with the ``\0`` as well. Think about that very carefully, because we'll be using **another byte of workspace**! And we might not need it, it's only any good for the "tail" case.
slink commented 2019-05-23 09:54:43 +00:00 (Migrated from code.uplex.de)
Copy link

Since we defined in varnish-cache that basically any data vmod handled is immutable (commit 2897c7e477ee70184189bed56bf74f24ebc3bb2a for example), we do not need to copy the subject argument to the match() function. We can even remove the existing WS_Copy(). It does not matter if the argument lives on the workspace or not, whoever created it needs to ensure that it has at least TASK lifetime.

But you are absolutely right that my patch as-is is wrong. Yes, If the WS_Copy() stayed, we would need to also copy the null byte.

I have updated the PR branch

Since we defined in varnish-cache that basically any data vmod handled is immutable (commit 2897c7e477ee70184189bed56bf74f24ebc3bb2a for example), we do not need to copy the `subject` argument to the `match()` function. We can even remove the existing `WS_Copy()`. It does not matter if the argument lives on the workspace or not, whoever created it needs to ensure that it has at least TASK lifetime. But you are absolutely right that my patch as-is is wrong. Yes, If the `WS_Copy()` stayed, we would need to also copy the null byte. I have updated the PR branch
slink commented 2019-05-30 18:40:07 +00:00 (Migrated from code.uplex.de)
Copy link

added 31 commits

  • a96736ac...2c43cb9e - 28 commits from branch uplex-varnish:master
  • 50b007da - add variable for clarity
  • a289ca4f - constify
  • 01c613af - micro-optimize backref when the reference is the subject tail

Compare with previous version

added 31 commits <ul><li>a96736ac...2c43cb9e - 28 commits from branch <code>uplex-varnish:master</code></li><li>50b007da - add variable for clarity</li><li>a289ca4f - constify</li><li>01c613af - micro-optimize backref when the reference is the subject tail</li></ul> [Compare with previous version](https://code.uplex.de/uplex-varnish/libvmod-re/merge_requests/2/diffs?diff_id=12&start_sha=a96736ac24ac76c3d2dbe032ade70a224add6cd4)
slink commented 2019-05-30 18:40:10 +00:00 (Migrated from code.uplex.de)
Copy link

added 1 commit

  • a5e55971 - we do not need to copy subject, it is immutable ...

Compare with previous version

added 1 commit <ul><li>a5e55971 - we do not need to copy subject, it is immutable ...</li></ul> [Compare with previous version](https://code.uplex.de/uplex-varnish/libvmod-re/merge_requests/2/diffs?diff_id=13&start_sha=01c613af3e88e04e4c85463e372a24e9dee8872a)
slink commented 2019-09-13 12:23:10 +00:00 (Migrated from code.uplex.de)
Copy link

merged

merged
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
uplex-varnish/libvmod-re!7
No description provided.