fix: patching namespaced attributes (#1049) #1061

iambumblehead · 2023-10-26T00:41:24Z

Node's runtime appeared to be unfarily optimising part the benchmark test for the original charCodeAt implementation. The final benchmark was updated to more closely resemble the actual behaviour of snabbdom. The final benchmark test and results are attached.

benchmark

// $ node ~/software/performance.js
const common = require('./node/benchmark/common.js');

const bench = common.createBenchmark(main, {
  n: [1000000000],
  type: [
    'charCodeAt',
    'charCodeAtV2'
  ],
  string: ['svg:test', 'xmlns:test', 'nonamespace']
});

function main(conf) {
  const string = conf.string
  const type = conf.type

  const set = (res, o = {}) => {
    o[res] = res
    return o
  }

  
  if (type === 'charCodeAt') {
    const colonChar = 58;

    bench.start();
    for (let i = conf.n, res; i--;) {
      if (string.charCodeAt(3) === colonChar)
        res = set('xml')
      else if (string.charCodeAt(5) === colonChar)
        res = set('xlink')
      else
        res = set('default')
    }
    bench.end(conf.n);
  }
  
  if (type === 'charCodeAtV2') {
    const colonChar = 58;
    const sChar = 115
    const mChar = 109
    
    bench.start();
    for (let i = conf.n, index, res; i--;) {
      if (string.charCodeAt(3) === colonChar)
        res = string.charCodeAt(0) === sChar ? set('svg') : set('xml')
      else if (string.charCodeAt(5) === colonChar)
        res = string.charCodeAt(1) === mChar ? set('xmlns') : set('xlink')
      else
        res = set('default')
    }
    bench.end(conf.n);
  }
}

$ node performance.js
 string="svg:test" type="charCodeAt"      n=1000000000: 361,036,563.2385391
 string="xmlns:test" type="charCodeAt"    n=1000000000: 275,365,679.2197518
 string="nonamespace" type="charCodeAt"   n=1000000000: 272,258,333.43921006
 string="svg:test" type="charCodeAtV2"    n=1000000000: 252,936,939.33049515
 string="xmlns:test" type="charCodeAtV2"  n=1000000000: 191,371,874.75000075
 string="nonamespace" type="charCodeAtV2" n=1000000000: 268,935,014.03115726

Feel free to give advice or criticism

iambumblehead · 2023-10-26T20:50:29Z

benchmarking shows that the approach used here is slow...

benchmark script

// $ node ~/software/performance.js
const common = require('./node/benchmark/common.js');

const bench = common.createBenchmark(main, {
  n: [10000000],
  type: ['charCodeAt','rev1_indexOf_split', 'rev2_indexOf_slice', 'rev3_indexOf_slice'],
  string: ['svg:test', 'xmlns:test', 'nonamespace']
});

function main(conf) {
  const string = conf.string
  const type = conf.type
  const legacyNamespaceQualifierMap = {
    svg: "http://www.w3.org/2000/svg",
    html: "http://www.w3.org/1999/xhtml",
    xml: "http://www.w3.org/XML/1998/namespace",
    xlink: "http://www.w3.org/1999/xlink",
    xmlns: "http://www.w3.org/2000/xmlns/",
  };  

  if (type === 'charCodeAt') {
    const colonChar = 58;

    bench.start();
    for (let i = conf.n, res; i--;) {
      if (string.charCodeAt(3) === colonChar)
        res = 'xml'
      else if (string.charCodeAt(5) === colonChar)
        res = 'xlink'
      else
        res = 'default'
    }
    bench.end(conf.n);
  }

  if (type === 'rev3_indexOf_slice') {
    bench.start();
    for (let i = conf.n, index, res; i--;) {
      index = string.indexOf(':', 2)
      res = index > -1
        ? legacyNamespaceQualifierMap[string.slice(0, index).toLowerCase()] || 'default'
        : 'default'
    }
    bench.end(conf.n);
  }
  
  if (type === 'rev2_indexOf_slice') {
    bench.start();
    for (let i = conf.n, index, res; i--;) {
      index = string.indexOf(':')
      res = index > -1
        ? legacyNamespaceQualifierMap[string.slice(0, index).toLowerCase()] || 'default'
        : 'default'
    }
    bench.end(conf.n);
  }

  if (type === 'rev1_indexOf_split') {
    bench.start();
    for (let i = conf.n, res; i--;) {
      if (string.indexOf(':') !== -1) {
        const namespaceQualifier = string.split(":")[0].toLowerCase();
        const namespaceUri = legacyNamespaceQualifierMap[namespaceQualifier];

        if (namespaceUri) {
          res = namespaceUri
        } else {
          res = 'default'
        }
      } else {
        res = 'default'
      }
    }
    bench.end(conf.n);
  }  
}

The results of the original and the results of version currently used in the PR

$ node performance.js type=charCodeAt
 string="svg:test" type="charCodeAt"            n=10000000: 484,433,058.54513997
 string="xmlns:test" type="charCodeAt"          n=10000000: 525,356,129.72660935
 string="nonamespace" type="charCodeAt"         n=10000000: 619,385,464.3238618

$ node performance.js type=rev1_indexOf_split
 string="svg:test" type="rev1_indexOf_split"    n=10000000: 3,843,110.7793953097
 string="xmlns:test" type="rev1_indexOf_split"  n=10000000: 3,788,721.676558171
 string="nonamespace" type="rev1_indexOf_split" n=10000000: 150,363,388.46361864

It is orders of magnitude slower. I tried optimising it this way

      index = string.indexOf(':', 2)
      res = index > -1
        ? legacyNamespaceQualifierMap[string.slice(0, index).toLowerCase()] || 'default'
        : 'default'

The results are better but still not great

$ node performance.js type=charCodeAt
 string="svg:test" type="charCodeAt"            n=10000000: 484,433,058.54513997
 string="xmlns:test" type="charCodeAt"          n=10000000: 525,356,129.72660935
 string="nonamespace" type="charCodeAt"         n=10000000: 619,385,464.3238618

node performance.js type=rev3_indexOf_slice
 string="svg:test" type="rev3_indexOf_slice"    n=10000000: 26,143,789.96112609
 string="xmlns:test" type="rev3_indexOf_slice"  n=10000000: 22,633,673.126783233
 string="nonamespace" type="rev3_indexOf_slice" n=10000000: 155,416,322.56197715

$ node performance.js type=rev1_indexOf_split
 string="svg:test" type="rev1_indexOf_split"    n=10000000: 3,843,110.7793953097
 string="xmlns:test" type="rev1_indexOf_split"  n=10000000: 3,788,721.676558171
 string="nonamespace" type="rev1_indexOf_split" n=10000000: 150,363,388.46361864

Maybe we could try using an approach that continues using charCodeAt and is less dynamic? cc @paldepind (feel free to ignore this, pinging you because this is generally interesting)

iambumblehead · 2023-10-26T21:01:44Z

      if (string.charCodeAt(3) === colonChar)
        res = string.startsWith('svg') ? 'svg' : 'xml'
      else if (string.charCodeAt(5) === colonChar)
        res = string.startsWith('xmlns') ? 'xmlns' : 'xlink'
      else if (string.charCodeAt(4) === colonChar)
        res = 'html'
      else
        res = 'default'

This one is better. The original one only handles two namepaces so it will always have fewer operations but I have few more ideas to try here...

$ node performance.js type=charCodeAt
 string="svg:test" type="charCodeAt"             n=10000000: 484,433,058.54513997
 string="xmlns:test" type="charCodeAt"           n=10000000: 525,356,129.72660935
 string="nonamespace" type="charCodeAt"          n=10000000: 619,385,464.3238618

$ node performance.js type=charCodeAt_expanded
 string="svg:test" type="charCodeAt_expanded"    n=10000000: 186,871,461.34361067
 string="xmlns:test" type="charCodeAt_expanded"  n=10000000: 66,944,352.734400325
 string="nonamespace" type="charCodeAt_expanded" n=10000000: 236,245,983.28673068

iambumblehead · 2023-10-26T21:10:08Z

      if (string.charCodeAt(3) === colonChar)
        res = string.charCodeAt(0) === sChar ? 'svg' : 'xml'
      else if (string.charCodeAt(5) === colonChar)
        res = string.charCodeAt(1) === mChar ? 'xmlns' : 'xlink'
      else if (string.charCodeAt(4) === colonChar)
        res = 'html'
      else
        res = 'default'

$ node performance.js type=charCodeAt
 string="svg:test" type="charCodeAt"              n=10000000: 484,433,058.54513997
 string="xmlns:test" type="charCodeAt"            n=10000000: 525,356,129.72660935
 string="nonamespace" type="charCodeAt"           n=10000000: 619,385,464.3238618

$ node performance.js type=charCodeAt_expanded2
 string="svg:test" type="charCodeAt_expanded2"    n=10000000: 338,774,997.0636677
 string="xmlns:test" type="charCodeAt_expanded2"  n=10000000: 247,869,671.7129792
 string="nonamespace" type="charCodeAt_expanded2" n=10000000: 232,190,851.40646937

iambumblehead · 2023-10-26T21:14:58Z

$ node performance.js type=charCodeAt
 string="svg:test" type="charCodeAt"                     n=10000000: 484,433,058.54513997
 string="xmlns:test" type="charCodeAt"                   n=10000000: 525,356,129.72660935
 string="nonamespace" type="charCodeAt"                  n=10000000: 619,385,464.3238618

$ node performance.js type=charCodeAt_expanded3_nohtml
 string="svg:test" type="charCodeAt_expanded3_nohtml"    n=10000000: 331,880,823.72023934
 string="xmlns:test" type="charCodeAt_expanded3_nohtml"  n=10000000: 253,863,085.3422704
 string="nonamespace" type="charCodeAt_expanded3_nohtml" n=10000000: 295,297,778.575213

$ node performance.js type=charCodeAt_expanded2
 string="svg:test" type="charCodeAt_expanded2"           n=10000000: 338,774,997.0636677
 string="xmlns:test" type="charCodeAt_expanded2"         n=10000000: 247,869,671.7129792
 string="nonamespace" type="charCodeAt_expanded2"        n=10000000: 232,190,851.40646937

iambumblehead · 2023-10-26T21:26:39Z

        (string.charCodeAt(3) === colonChar)
        ? (string.charCodeAt(0) === sChar ? 'svg' : 'xml')
        : (string.charCodeAt(5) === colonChar)
        && (string.charCodeAt(1) === mChar ? 'xmlns' : 'xlink')
        || 'default'

results vary on each run but this one does seem to return results that closest to the original

$ node performance.js type=charCodeAt
 string="svg:test" type="charCodeAt"                     n=10000000: 484,433,058.54513997
 string="xmlns:test" type="charCodeAt"                   n=10000000: 525,356,129.72660935
 string="nonamespace" type="charCodeAt"                  n=10000000: 619,385,464.3238618

$ node performance.js type=charCodeAt_expanded4
 string="svg:test" type="charCodeAt_expanded4"           n=10000000: 352,228,849.56457293
 string="xmlns:test" type="charCodeAt_expanded4"         n=10000000: 251,956,848.05611262
 string="nonamespace" type="charCodeAt_expanded4"        n=10000000: 303,769,087.86223173

iambumblehead · 2023-10-26T21:27:47Z

let's amend the commit with this latest version

iambumblehead · 2023-10-26T22:12:20Z

the most recent benchmark script and results are attached at the top of this PR discussion

paldepind · 2023-11-07T17:18:28Z

To make sure I understand correctly: The attributes module currently handles attributes that begin with "xml:" and "xlink:". As mentioned in #1049 attributes that begin with "xmlns:" are not handled correctly.

According to the linked SO answer, there are also attributes that begin with html and svg. Do we have some other source for the existence of these attributes in addition to the SO answer? I searched a bit and could not find examples of such attributes. As the issue in question only mentions an "xmlns:" attribute I would be inclined to only add support for that one unless we can gather some good proof that the "html:" and "svg:" attributes are also used.

src/modules/attributes.ts

paldepind · 2023-11-07T17:24:10Z

Please see the comment I've left. If we decide that "svg:" and "html:" attributes exist and are important then that bug should be fixed (and probably tests should be added as well for those prefixes). Otherwise, we can just remove the handling of "svg:" and "html:" and the check that starts by seeing whether the first character is "x" will still be valid.

src/modules/attributes.ts

iambumblehead · 2023-11-07T18:36:02Z

The stack overflow link maps the following namespaces and uris: link,

const NAMESPACES = {
  svg: 'http://www.w3.org/2000/svg',
  html: 'http://www.w3.org/1999/xhtml',
  xml: 'http://www.w3.org/XML/1998/namespace',
  xlink: 'http://www.w3.org/1999/xlink',
  xmlns: 'http://www.w3.org/2000/xmlns/' // sic for the final slash...
}

namespaces currently supported in the master branch are: xml and xlink. The solution for the issue linked to this PR requires support for xmlns.

I'll look for information about svg and html and if I find anything that supports their inclusion in this PR, I will link to that here and include them in the PR and otherwise remove them from the PR

iambumblehead · 2023-11-07T19:12:50Z

Google search results return only few references to the html namespace url and it seems that url is used only with the root html element this way, and using it declares an xml-style document rather than the typical html-style document <!DOCTYPE html> and probably does not need to be supported by snabbdom

<html xmlns="http://www.w3.org/1999/xhtml">

The svg url is svg-specific and there are not many search results for the svg url either, but these stackoverflow links indicate the url is used with xml namespace to preserve newlines and spaces in text they render link 1, link-2

elm.setAttributeNS("http://www.w3.org/XML/1998/namespace", "xml:space","preserve");
elm.text(`some   test with spaces   and

newlines`);

@paldepind should the svg url be supported? Do you also think it is okay to skip support for the html namespace url?

iambumblehead · 2023-11-07T19:57:40Z

let's remove the svg condition and maybe add it later through a separate PR, if that is wanted

iambumblehead · 2023-11-07T20:03:01Z

This patch adds support for the svg namespace, here for reference in case it is added later

diff --git a/src/modules/attributes.ts b/src/modules/attributes.ts
index c82f918..a481a15 100755
--- a/src/modules/attributes.ts
+++ b/src/modules/attributes.ts
@@ -6,6 +6,7 @@ export type Attrs = Record<string, string | number | boolean>;
 const xlinkNS = "http://www.w3.org/1999/xlink";
 const xmlnsNS = "http://www.w3.org/2000/xmlns/";
 const xmlNS = "http://www.w3.org/XML/1998/namespace";
+const svgNS = "http://www.w3.org/2000/svg";
 const colonChar = 58;
 const xChar = 120;
 const mChar = 109;
@@ -31,8 +32,10 @@ function updateAttrs(oldVnode: VNode, vnode: VNode): void {
       } else if (cur === false) {
         elm.removeAttribute(key);
       } else if (key.charCodeAt(3) === colonChar) {
-        // Assume 'xml' namespace
-        elm.setAttributeNS(xmlNS, key, cur as any);
+        // Assume 'xml' or 'svg' namespace
+        key.charCodeAt(1) === mChar
+          ? elm.setAttributeNS(xmlNS, key, cur as any)
+          : elm.setAttributeNS(svgNS, key, cur as any);
       } else if (key.charCodeAt(5) === colonChar) {
         // Assume 'xmlns' or 'xlink' namespace
         key.charCodeAt(1) === mChar

paldepind

Thanks for looking into this in more detail. From this, I think its reasonable to only extend the support to "xmlns:" attributes.

The PR looks good except for the one comment I've left.

src/modules/attributes.ts

iambumblehead · 2023-11-08T18:39:51Z

the original if/else arrangement is restored so that the diff is as small and specific as possible

paldepind · 2023-11-08T18:52:29Z

Superb! Thanks a lot for the PR and the surrounding research.

paldepind reviewed Nov 7, 2023

View reviewed changes

src/modules/attributes.ts Outdated Show resolved Hide resolved

paldepind reviewed Nov 7, 2023

View reviewed changes

src/modules/attributes.ts Outdated Show resolved Hide resolved

paldepind mentioned this pull request Nov 7, 2023

Issue with attribute.js - setAttributeNS #1049

Closed

iambumblehead requested a review from paldepind November 7, 2023 20:04

paldepind requested changes Nov 8, 2023

View reviewed changes

src/modules/attributes.ts Outdated Show resolved Hide resolved

fix: patching namespaced attributes (#1049)

438b9fd

iambumblehead requested a review from paldepind November 8, 2023 18:40

paldepind merged commit 8c67f42 into snabbdom:master Nov 8, 2023
1 check passed

iambumblehead deleted the add-support-legacy-namespace-attribute-patching branch November 8, 2023 21:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: patching namespaced attributes (#1049) #1061

fix: patching namespaced attributes (#1049) #1061

iambumblehead commented Oct 26, 2023 •

edited

iambumblehead commented Oct 26, 2023 •

edited

iambumblehead commented Oct 26, 2023

iambumblehead commented Oct 26, 2023

iambumblehead commented Oct 26, 2023

iambumblehead commented Oct 26, 2023

iambumblehead commented Oct 26, 2023

iambumblehead commented Oct 26, 2023

paldepind commented Nov 7, 2023

paldepind commented Nov 7, 2023

iambumblehead commented Nov 7, 2023

iambumblehead commented Nov 7, 2023 •

edited

iambumblehead commented Nov 7, 2023

iambumblehead commented Nov 7, 2023

paldepind left a comment

iambumblehead commented Nov 8, 2023

paldepind commented Nov 8, 2023

fix: patching namespaced attributes (#1049) #1061

fix: patching namespaced attributes (#1049) #1061

Conversation

iambumblehead commented Oct 26, 2023 • edited

iambumblehead commented Oct 26, 2023 • edited

iambumblehead commented Oct 26, 2023

iambumblehead commented Oct 26, 2023

iambumblehead commented Oct 26, 2023

iambumblehead commented Oct 26, 2023

iambumblehead commented Oct 26, 2023

iambumblehead commented Oct 26, 2023

paldepind commented Nov 7, 2023

paldepind commented Nov 7, 2023

iambumblehead commented Nov 7, 2023

iambumblehead commented Nov 7, 2023 • edited

iambumblehead commented Nov 7, 2023

iambumblehead commented Nov 7, 2023

paldepind left a comment

Choose a reason for hiding this comment

iambumblehead commented Nov 8, 2023

paldepind commented Nov 8, 2023

iambumblehead commented Oct 26, 2023 •

edited

iambumblehead commented Oct 26, 2023 •

edited

iambumblehead commented Nov 7, 2023 •

edited