Skip to content

esi: replace _findOpeningTag with memchr #13173

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

Open
pixitha wants to merge 3 commits into
base: master
Choose a base branch
from
+112 −55
Open

esi: replace _findOpeningTag with memchr #13173

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2445c39
esi: replace _findOpeningTag with memchr
May 18, 2026
fdf09bf
fixing style issue
pixitha May 18, 2026
6f60f18
esi: inline ESI_TAG_PREFIX_LEN and HTML_COMMENT_NODE_INFO.tag_suffix_len
May 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 40 additions & 55 deletions plugins/esi/lib/EsiParser.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <ts/ts.h>

#include <cctype>
#include <cstring>

using std::string;
using namespace EsiLib;
Expand Down Expand Up @@ -182,68 +183,52 @@ EsiParser::_compareData(const string &data, size_t pos, const char *str, int str
return PARTIAL_MATCH;
}

/** This implementation is optimized but not completely correct. If
* the opening tag were to have a repeating opening sequence ('<e<esi'
* or something like that), this will break. However that is not the
* case for the two opening tags we are looking for */
/** Uses memchr to skip non-'<' bytes, then memcmp to verify each candidate
* anchor. Delegates scanning to the platform's optimized memchr
* implementation. Does not have the KMP-failure limitation of the original
* state-machine. */
EsiParser::MATCH_TYPE
EsiParser::_findOpeningTag(const string &data, size_t start_pos, size_t &opening_tag_pos, bool &is_html_comment_node) const
{
size_t i_data = start_pos;
int i_esi = 0, i_html_comment = 0;

while (i_data < data.size()) {
if (data[i_data] == ESI_TAG_PREFIX[i_esi]) {
if (++i_esi == ESI_TAG_PREFIX_LEN) {
is_html_comment_node = false;
opening_tag_pos = i_data - i_esi + 1;
const char *const buf = data.data();
Copy link
Copy Markdown
Contributor

@zwoop zwoop May 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

One thought here, and I asked Claude to make an example: Did you consider pulling in and using libswoc TextView instead? It excels at things like "string parsing". Not a show stopper, and could be a future consideration.

I asked Claude to write a suggested use with TextView (not tested ...)

swoc::TextView view{data.data() + start_pos, data.size() - start_pos};
const swoc::TextView esi_prefix{ESI_TAG_PREFIX, ESI_TAG_PREFIX_LEN};
const swoc::TextView html_prefix{HTML_COMMENT_NODE_INFO.tag_suffix,
                                 size_t(HTML_COMMENT_NODE_INFO.tag_suffix_len)};
while (!view.empty()) {
  size_t pos = view.find('<');
  if (pos == swoc::TextView::npos) return NO_MATCH;
  view.remove_prefix(pos);
  size_t abs_pos = data.size() - view.size();
  if (view.starts_with(esi_prefix)) { /* COMPLETE_MATCH esi */ }
  if (view.size() > html_prefix.size() && view.starts_with(html_prefix)) {
    char ch = view[html_prefix.size()];
    if (ch==' '||ch=='\t'||ch=='\r'||ch=='\n') { /* COMPLETE_MATCH html */ }
  }
  if (view.size() < esi_prefix.size() && esi_prefix.starts_with(view)) { /* PARTIAL */ }
  if (view.size() <= html_prefix.size() && html_prefix.starts_with(view)) { /* PARTIAL */ }
  view.remove_prefix(1);
}

Copy link
Copy Markdown
Author

@pixitha pixitha May 21, 2026

Choose a reason for hiding this comment

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

I was mostly unaware of what was offered in libswoc, so that's def why I didn't use it. I did some quick benchmarks with a synthetic comparison between this vs TextView and libswoc is as fast if not slightly faster sometimes. Its within margin of error, so Its more up to you guys if you want it re-written that way now, or something we should tackle later?

const size_t total = data.size();
const size_t esi_len = ESI_TAG_PREFIX_LEN;
const size_t hlen = HTML_COMMENT_NODE_INFO.tag_suffix_len;
Copy link
Copy Markdown
Contributor

@zwoop zwoop May 19, 2026

Choose a reason for hiding this comment

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

Do you really need this hlen local? The compiler will optimize it away, but I honestly would just like to see HTML_COMMENT_NODE_INFO.tag_suffix_len where you need this, specially since you memcmp from HTML_COMMENT_NODE_INFO.tag_suffix already (no shadow variable for the string).

Same with the esi_len I think, neither are blockers.

Copy link
Copy Markdown
Author

@pixitha pixitha May 19, 2026

Choose a reason for hiding this comment

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

corrected in latest commit

size_t i = start_pos;

while (i < total) {
const char *p = static_cast<const char *>(memchr(buf + i, '<', total - i));
if (!p) {
return NO_MATCH;
}
const size_t pos = static_cast<size_t>(p - buf);
const size_t avail = total - pos;

if (avail >= esi_len && memcmp(p, ESI_TAG_PREFIX, esi_len) == 0) {
is_html_comment_node = false;
opening_tag_pos = pos;
return COMPLETE_MATCH;
}
// hlen+1 bytes needed: hlen for the tag, 1 for the required trailing whitespace
if (avail > hlen && memcmp(p, HTML_COMMENT_NODE_INFO.tag_suffix, hlen) == 0) {
const char ch = p[hlen];
if (ch == ' ' || ch == '\t' || ch == '\r' || ch == '\n') {
is_html_comment_node = true;
opening_tag_pos = pos;
return COMPLETE_MATCH;
}
} else {
if (i_esi) {
i_esi = 0;
--i_data; // we do this to reexamine the current char as target string might start from here
if (i_html_comment) {
--i_html_comment; // in case other target string has started matching, adjust it's index
}
}
}
// doing the exact same thing for the other target string
if (i_html_comment < HTML_COMMENT_NODE_INFO.tag_suffix_len &&
data[i_data] == HTML_COMMENT_NODE_INFO.tag_suffix[i_html_comment]) {
if (++i_html_comment == HTML_COMMENT_NODE_INFO.tag_suffix_len && i_data + 1 < data.size()) {
char ch = data[i_data + 1]; //<!--esi must follow by a space char
if (ch == ' ' || ch == '\t' || ch == '\r' || ch == '\n') {
is_html_comment_node = true;
opening_tag_pos = i_data - i_html_comment + 1;
return COMPLETE_MATCH;
}
}
} else {
if (i_html_comment) {
i_html_comment = 0;
--i_data; // same comments from above applies
if (i_esi) {
--i_esi;
}
}
if (avail < esi_len && memcmp(p, ESI_TAG_PREFIX, avail) == 0) {
is_html_comment_node = false;
opening_tag_pos = pos;
return PARTIAL_MATCH;
}
++i_data;
}
// partial matches; with the nature of our current opening tags, the
// only way we can have a partial match for both target strings is
// if the last char of the input string is '<' and that is not
// enough information to differentiate the tags; Anyway, the parser
// takes no action for a partial match
if (i_esi) {
is_html_comment_node = false;
opening_tag_pos = i_data - i_esi;
return PARTIAL_MATCH;
}
if (i_html_comment) {
is_html_comment_node = true;
opening_tag_pos = i_data - i_html_comment;
return PARTIAL_MATCH;
if (avail <= hlen && memcmp(p, HTML_COMMENT_NODE_INFO.tag_suffix, avail) == 0) {
is_html_comment_node = true;
opening_tag_pos = pos;
return PARTIAL_MATCH;
}
i = pos + 1;
}
return NO_MATCH;
}
Expand Down
72 changes: 72 additions & 0 deletions plugins/esi/test/parser_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -986,6 +986,78 @@ TEST_CASE("esi parser test")
REQUIRE(node_list.size() == 6);
}

SECTION("chunk boundary: <esi: prefix exact at chunk end")
{
// avail == ESI_TAG_PREFIX_LEN (5) at end of first chunk — exercises the
// avail >= esi_len complete-match branch with nothing left in the buffer.
DocNodeList node_list;
REQUIRE(parser.parseChunk("pre<esi:", node_list) == true);
REQUIRE(parser.parseChunk("include src=url/>", node_list) == true);
REQUIRE(parser.completeParse(node_list) == true);
REQUIRE(node_list.size() == 2);
auto it = node_list.begin();
REQUIRE(it->type == DocNode::TYPE_PRE);
REQUIRE(it->data_len == 3);
REQUIRE(strncmp(it->data, "pre", 3) == 0);
++it;
REQUIRE(it->type == DocNode::TYPE_INCLUDE);
REQUIRE(it->attr_list.size() == 1);
check_node_attr(it->attr_list.front(), "src", "url");
}

SECTION("chunk boundary: <!--esi prefix exact at chunk end")
{
// avail == hlen (7) at end of first chunk — exercises the avail <= hlen
// partial-match branch; the trailing whitespace arrives in the next chunk.
DocNodeList node_list;
REQUIRE(parser.parseChunk("pre<!--esi", node_list) == true);
REQUIRE(parser.parseChunk(" <esi:comment text=blah/>-->", node_list) == true);
REQUIRE(parser.completeParse(node_list) == true);
REQUIRE(node_list.size() == 2);
auto it = node_list.begin();
REQUIRE(it->type == DocNode::TYPE_PRE);
REQUIRE(it->data_len == 3);
REQUIRE(strncmp(it->data, "pre", 3) == 0);
++it;
REQUIRE(it->type == DocNode::TYPE_HTML_COMMENT);
REQUIRE(it->data_len == static_cast<int>(strlen("<esi:comment text=blah/>")));
REQUIRE(strncmp(it->data, "<esi:comment text=blah/>", it->data_len) == 0);
}

SECTION("<!--esi without trailing whitespace is not a tag")
{
// <!--esi_ has no whitespace after the prefix — must be treated as PRE
// and scanning must continue to find the real <esi: tag that follows.
DocNodeList node_list;
REQUIRE(parser.parse(node_list, "<!--esi_nospace<esi:include src=url/>") == true);
REQUIRE(node_list.size() == 2);
auto it = node_list.begin();
REQUIRE(it->type == DocNode::TYPE_PRE);
REQUIRE(it->data_len == static_cast<int>(strlen("<!--esi_nospace")));
REQUIRE(strncmp(it->data, "<!--esi_nospace", it->data_len) == 0);
++it;
REQUIRE(it->type == DocNode::TYPE_INCLUDE);
REQUIRE(it->attr_list.size() == 1);
check_node_attr(it->attr_list.front(), "src", "url");
}

SECTION("multiple false '<' anchors before valid tag")
{
// Exercises the memchr loop iterating past several non-tag '<' bytes
// before landing on a real <esi: tag. All skipped content becomes PRE.
DocNodeList node_list;
REQUIRE(parser.parse(node_list, "< <a> <!--esi_bad <esi:include src=url/>") == true);
REQUIRE(node_list.size() == 2);
auto it = node_list.begin();
REQUIRE(it->type == DocNode::TYPE_PRE);
REQUIRE(it->data_len == static_cast<int>(strlen("< <a> <!--esi_bad ")));
REQUIRE(strncmp(it->data, "< <a> <!--esi_bad ", it->data_len) == 0);
++it;
REQUIRE(it->type == DocNode::TYPE_INCLUDE);
REQUIRE(it->attr_list.size() == 1);
check_node_attr(it->attr_list.front(), "src", "url");
}

SECTION("No handler attr")
{
string input_data = "<esi:special-include />";
Expand Down