[jwrosewell]

Finding Properties in both on-premise Engine specifications said every read MUST use the position. Three implementations read by name, because each measured a read by name as faster than a read by position, and the release rule is that the fastest approach is the one used. This draft makes the rule a SHOULD, gives the exception as a MAY that an implementation's own tests have to earn, names which reads go by name in each implementation, and links each measurement and the issues that follow it up.

This draft merges into rc together with 51degrees-php#72, because the PHP items are true only once #72 is in. Until then PHP reads by position on rc, since 51degrees-php#66.

What James decided

Asked at about 07:10 UTC on 12 November whether the exception to "Every read MUST use the position" should stand, James Rosewell said this.

Change the spec to a SHOULD but allow for exceptions when tests prove it.

So the head before that decision, 5db7141, took the wrong shape. It kept the MUST and carved an exception out of it. This head makes the rule itself a SHOULD, and makes the exception a MAY that only a measurement from a test or a continuous integration run can earn.

The problem

At d72eb07, the rc head this branch starts from, device-detection-on-premise.md lines 202 to 209 says the following, and ip-intelligence-on-premise.md lines 408 to 415 says the same with IpRangeStart as the example.

• Every read MUST use the position. A typed accessor, such as IsMobile, resolves its position once, when the Engine is created or when its data object is first made, and reads by position from then on. A read by name looks the name up in a table from the names the Engine was created with to their positions, and then reads by position. That table is built once, when the Engine is created, from the names the native code gives, for example through the Engine's getRequiredProperties, and MUST NOT be built again from a later data set.

Three implementations read by name instead.

1. Node.js reads every value by name in both Engines since 51degrees-node#68, merged into rc as f3f89811. Each getter in its SWIG wrapper is a dispatcher that tries the overload taking a name first, and given a number that attempt fails and makes a V8 Error, which captures a stack trace, before the overload taking an int runs. In job 130437032538 of run 42995016804 a native read of IsMobile took 711 ns by name against 2,797 ns by index. The measurement was taken with the Device Detection Engine only. The read by index, its table and its tests were taken out in 927114d4.
2. .NET Device Detection reads by name, as "Device Detection stays by name" in 51degrees-dotnet#133, merged as 2b5c9eda, sets out. The tryGet fast paths in hash_csharp.i take a name and return the value in one call, where the Value<T> getters return a value object, which costs an allocation and three more calls. In job 130391855953, on the TAC file with five names and MaxPerformance, a tryGet read of IsMobile took 139.7 ns by name against 207.8 ns for a Value<T> read by index. A Value<T> read by name took 236.9 ns, so that kind of read is slower by name. .NET IP Intelligence reads by index.
3. PHP reads every value by name in both Engines in 51degrees-php#72, an open draft at c6b416d3. A read already finds the property's spelling among the names the engine was built with, so reading by index saves only the native lookup of the name, and finding the index in PHP costs more than that lookup. Run 43089315389 timed whole requests with the Device Detection TAC file, 3,000 requests a figure over 10 rounds. Its job log prints these medians, and "index faster than name in 0 of 10 rounds" for each size. IP Intelligence was not timed.

Names read	By index	By name
1	63,537 ns	60,904 ns
5	108,651 ns	101,801 ns
15	214,271 ns	196,263 ns

How SHOULD and MUST are written here

stylistic-conventions.md lines 72 to 77 at d72eb074 says this.

RFC 2119

RFC 2119 terms are UPPER CASE when used with the meaning defined in
RFC 2119. Avoid use of the same terms where they do not have the RFC 2119 meaning.

MUST, SHOULD, MUST NOT, SHOULD NOT, MAY etc.

So SHOULD, MUST, MUST NOT and MAY are upper case in the new text, and the new text uses no lower case "should", "may" or "must" anywhere, as the second sentence of that convention asks. RFC 8174, which updates RFC 2119 and does not obsolete it, agrees that only the upper case forms carry the meaning.

The two terms are used in the sense RFC 2119 section 3 and section 5 give, which is that SHOULD means "there may exist valid reasons in particular circumstances to ignore a particular item, but the full implications must be understood and carefully weighed before choosing a different course", and MAY means "an item is truly optional". A measurement from the implementation's own tests is the valid reason, and weighing it is reading the run.

What changes

Both files change in the same words, each naming the other Engine where it needs to.

The first item's opening sentence becomes a SHOULD, and the inline "except ..." clause the previous head carried is replaced by a plain pointer to the next item, as a SHOULD already carries its own exception and does not need one carved out of it. The rest of that item, from "A typed accessor" to "from a later data set", is word for word as it is on rc, so the typed accessor resolving its position once, and the table built once and never built again from a later data set, stay as they are. That paragraph is wrapped again to the file's width, because the previous head left a short line in the middle of it, and no word in it changed.

The second item becomes a MAY, with the proof required to be a measurement a test or a continuous integration run takes and records. A run on a developer's machine written up in prose does not count, and the implementation records the run in the change that makes the read go by name. The rest of the section is untouched.

The new text in device-detection-on-premise.md, lines 202 to 261 at 9c4822f4, is this.

• Every read SHOULD use the position. The next item gives the exception, which is a read an implementation's tests prove is faster by name. A typed accessor, such as IsMobile, resolves its position once, when the Engine is created or when its data object is first made, and reads by position from then on. A read by name looks the name up in a table from the names the Engine was created with to their positions, and then reads by position. That table is built once, when the Engine is created, from the names the native code gives, for example through the Engine's getRequiredProperties, and MUST NOT be built again from a later data set.
• An implementation MAY read by name where its own tests prove that a read by name is faster than a read by position, as the fastest approach is the one used. The proof MUST be a measurement a test or a continuous integration run takes and records, not a run on a developer's machine written up in prose, and the implementation MUST record that run in the change that makes the read go by name. Each read by name is listed here, with the change that made it and, where there is one, the run that proves it. These implementations read by name.
  • Node.js, every read in this Engine and the IP Intelligence Engine. Its timing tests prove a read by name faster, in 51degrees-node#68. Each getter in its SWIG wrapper tries the form that takes a name before the form that takes a position, so a read by position first fails and makes a JavaScript error, which costs more than the native code's lookup of the name. In job 130437032538 a read of IsMobile took 711 ns by name and 2,797 ns by position. That job is red because the only two tests that failed in it are timing assertions, and a read by position being slower is what made them fail. The measurement was taken with this Engine, and timing the IP Intelligence Engine on its own is 51degrees-node#81.
  • .NET, the TryGet reads in this Engine. Its cost timing run proves a read by name faster, in 51degrees-dotnet#133. These are the fast getters in its SWIG wrapper for a bool, integer, double or string value, which take a name and return the value in one call. In job 130391855953, with the TAC file and MaxPerformance, a TryGet read of IsMobile took 139.7 ns by name, where a read by position, through a Value<T> getter, took 207.8 ns. Its other reads, being list values and the read a TryGet read falls back to for the no-value message, use the Value<T> getters, which return a value object at the cost of an allocation and three more calls. Those reads also go by name, with no test proving them faster that way, as the same job measured a Value<T> read of IsMobile as slower by name, 236.9 ns against 207.8 ns by position. They are therefore outside this exception, and moving them to the position is 51degrees-dotnet#139. List reads were not timed. Its IP Intelligence Engine reads by position.
  • PHP, every read in this Engine and the IP Intelligence Engine. Its request timing run proves a read by name faster, in 51degrees-php#72. Finding a position in PHP costs more than the native code's lookup of the name. In run 43089315389, with the TAC file, the median request reading 1, 5 and 15 Properties took 60.9, 101.8 and 196.3 µs by name against 63.5, 108.7 and 214.3 µs by position, and reading by position was faster in none of its 30 rounds. The measurement was taken with this Engine, and timing the IP Intelligence Engine on its own is 51degrees-php#75.

ip-intelligence-on-premise.md, lines 408 to 450 at 9c4822f4, has the same two items with IpRangeStart as the example, and lists Node.js and PHP as follows.

• Node.js, every read in this Engine and the Device Detection Engine. Its timing tests prove a read by name faster, in 51degrees-node#68, where they ran against the Device Detection Engine. Each getter in its SWIG wrapper, this Engine's weighted getters among them, tries the form that takes a name before the form that takes a position, so a read by position first fails and makes a JavaScript error, which costs more than the native code's lookup of the name. In job 130437032538 a read of the Device Detection Property IsMobile took 711 ns by name and 2,797 ns by position. That job is red because the only two tests that failed in it are timing assertions, and a read by position being slower is what made them fail. Timing this Engine on its own is 51degrees-node#81.
• PHP, every read in this Engine and the Device Detection Engine. Its request timing run proves a read by name faster, in 51degrees-php#72, where it ran against the Device Detection Engine, as the change made both Engines read in the same way. Finding a position in PHP costs more than the native code's lookup of the name. In run 43089315389, with the Device Detection TAC file, the median request reading 1, 5 and 15 Properties took 60.9, 101.8 and 196.3 µs by name against 63.5, 108.7 and 214.3 µs by position, and reading by position was faster in none of its 30 rounds. Timing this Engine on its own is 51degrees-php#75.

What the previous head said

For comparison, device-detection-on-premise.md lines 202 to 216 at 5db7141b read as follows, and ip-intelligence-on-premise.md lines 408 to 422 at 5db7141b read the same.

• Every read MUST use the position, except a read that the implementation's own measurement shows is faster by name, as the next item describes. A typed accessor, such as IsMobile, resolves its position once, ...
• A read that an implementation's own measurement shows is faster by name than by position goes by name, as the fastest approach is the one used. The implementation MUST record that measurement, with the runs that took it, in the change that makes the read go by name, and it is listed here with a link to that change. These implementations read by name.

Three things changed from those words. The rule is a SHOULD rather than a MUST with a hole in it. The exception is a MAY an implementation takes rather than a statement that a faster read "goes by name". And the proof is named, being a test or a continuous integration run that takes and records the measurement, where the previous wording accepted any "measurement".

Which reads go by name

Each item says which reads go by name, as checked in the code.

• Node.js passes property.name to every native getter in getInternal (Device Detection swigData.js lines 142 to 157 and IP Intelligence lines 105 to 155, at f76d8b05), so every read goes by name.
• .NET Device Detection reads a bool through GetValueAsBool(string propertyName), which calls TryGetBoolFast(propertyName, ...) and, only when that finds no value, getValueAsBool(propertyName) to recover the no-value message (DeviceDataHash.cs lines 294 to 322 at 875acebb), and integers, doubles, strings and JavaScript in the same way. A list value goes through GetValues(string propertyName), which calls the Value<T> getter getValues(propertyName) (lines 378 to 390). So only the TryGet reads have a test proving them faster by name, and the item now says in those words that the Value<T> reads go by name without one, which puts them outside the exception, and that 51degrees-dotnet#139 moves them.
• In PHP, 51degrees-php#72 says "Both SwigData classes hand each accessor the name findProperty gives", so every read goes by name.

The implementations that read by position

Each was checked in its pull requests, and every merge commit named here is in its repository's rc.

Implementation	Device Detection	IP Intelligence	Where
Java	position	position	51degrees-java#101 for an engine built with names and 51degrees-java#82 for one built with none, both merged
Python	position	position	51degrees-python#93 and 51degrees-python#102, both merged
Go	position	position	51degrees-go#96 and 51degrees-go#98, both merged, for Device Detection, and both say IP Intelligence already reads by position. 51degrees-go#102, an open draft, keeps the IP Intelligence table across reloads
Rust	position	position	51degrees-rust#376, merged into release/dotnet-parity and brought into rc by 51degrees-rust#325
.NET	TryGet reads by name, listed. Value<T> reads by name with no test behind them, 51degrees-dotnet#139	position	51degrees-dotnet#133, merged, for an engine built with names, and 51degrees-dotnet#137, an open draft, for one built with none
Node.js	name, listed	name, listed, 51degrees-node#81 to time it	51degrees-node#68, merged
PHP	name, listed	name, listed, 51degrees-php#75 to time it	51degrees-php#72, an open draft. Until it merges, rc reads by position since 51degrees-php#66

Checks

Both checks on the head 9c4822f passed, and the headSha of each run was read back as that commit. The repository has nothing to build (ci/build-project.ps1 prints "Nothing to build"), so these are its only checks.

• UTM link lint, run 43177438989, success. The change adds no 51degrees.com link.
• Only rc into main, run 43177439393, success.

Every figure in the new text was read again from its job's log.

• Node.js, job 130437032538 (Ubuntu ARM, Node 22), prints "Native read of IsMobile 20000 times, by name 711 ns a read, by index 2797 ns a read". Both that job and its run 42995016804 are red. The job's only failing step is "Build and Test", and the only two tests that failed in it are "a read by index is not slower than a read by name" and "an Element Data read is not slower than it was", so the measurement itself is what made the job red. The later green build of 51degrees-node#68, run 43016967869 on 00cb4f67, prints no timing at all in either of its Node 22 jobs, 130510472189 and 130510472237, so this red job is the only published record of the figure.
• .NET, job 130391855953, prints for the TAC file, MaxPerformance and five names RawFastBool ns|139.7, RawNameBool ns|236.9 and RawIndexBool ns|207.8. It prints no figure for a list read.
• PHP, the one job of run 43089315389, prints the table above and "index faster than name in 0 of 10 rounds" for 1, 5 and 15 names.

History of this draft

The first head, db72e2c, listed whole implementations as reading by name and left two points open, which were whether Node.js and PHP should time IP Intelligence on their own, and whether the .NET reads with no fast path should go by position. James decided on 11 November that both go to next week, and the issues above follow them up.

The second head, 1d40cc8, makes the exception cover a read rather than a whole implementation, says only the .NET TryGet reads are measured faster by name, and links the three issues.

The third head, 5db7141, answers a review point on the second. Both pages linked the Node.js measurement as run 42995016804, whose conclusion is failure, so a reader of the published page who opened the link saw a red run and no figure. Each page now links job 130437032538 inside that run, which is where the figure is printed, in the same form the .NET item already used. That job is red as well, so each page also says in one sentence why.

The fourth head, e3097b4, is the change made to James's decision above, turning the MUST into a SHOULD and the carved-out exception into a MAY. The title of the draft changed with it, from "Read by name where an implementation measures it as faster than by position", because the draft is now about the strength of the rule as much as about the three implementations that read by name.

The fifth head, 9c4822f, is this one, and answers a review point on the fourth. The lead-in to the list promised "the change that made it and the run that proves it" for every read by name, which the .NET entry then contradicted four lines later, as the Value<T> reads are listed and have no run behind them. The lead-in now reads "and, where there is one, the run that proves it", which holds for all three entries. The sentence is the same on both pages, so both moved together.

What remains

1. PHP. This draft merges into rc together with 51degrees-php#72, as above.
2. IP Intelligence timing, next week, in 51degrees-node#81 and 51degrees-php#75. Node.js's IP Intelligence wrapper declares the overload taking a name first as well (src/ipi/ResultsIpi.i lines 65 to 68 at 7c4ac436).
3. .NET Value<T> and list reads, next week, in 51degrees-dotnet#139. Those reads now sit outside the exception in the new text, so the .NET item changes when they move.
4. When a read by position may return. 51degrees-node#68 gives James two options for next week, each needing the SWIG wrappers generated again, after which its read by position can come back with its timing tests. 51degrees-dotnet#133 says Device Detection could read by position as fast only with index forms of the four fast paths in the native interface. Either change, with a test showing the position faster, takes those reads off the list.

[jwrosewell]

Closing and reopening at once to start the checks again, because the runs queued during GitHub's Actions outage this evening never started.