Repository navigation
Move cxx to the 51degrees-cxx release 4.5.105 and read by index on an engine built with no names - #82
Merged
Merged
Conversation
The configuration's collections are borrowed. Test that each one the configuration hands out is borrowed, and stop forcing the flag by reflection in the two tests that read them.
SWIG 4.5.1 now marks the configuration's collection getters as borrowed, as the corrected MetaData.i asks. The Device Detection wrapper is unchanged apart from comments recording the include path, so it is left as it was.
Takes the eight cxx commits after 865cee97, from 51Degrees/51degrees-cxx#118, 51Degrees/51degrees-cxx#139, 51Degrees/51degrees-cxx#143, 51Degrees/51degrees-cxx#136, 51Degrees/51degrees-cxx#131, 51Degrees/51degrees-cxx#141, 51Degrees/51degrees-cxx#144 and 51Degrees/51degrees-cxx#145.
Takes the two cxx commits after b4f769ad, from 51Degrees/51degrees-cxx#130 and 51Degrees/51degrees-cxx#146. This is the cxx commit for the release.
This was referenced Nov 11, 2026
…cee97 # Conflicts: # cxx
An engine built with no names should take its names from the native engine after the first data file loads, give every property of that file an index the native results agree with, and keep those indexes through a reload from a file whose properties are a superset in another order. The read by name against the read by index is measured for such an engine as it is for one built with names.
51degrees-cxx 7c4ac436 keeps the first data file's property names, in their order, across every reload of an engine created with no names, and its C++ reads by index work on such an engine. The engine therefore makes its map of names to indexes once, from the native engine's required properties after the first data file loads, and never again. A property only a later data file has is not in the map, so it reads by name and gives no value until the engine is built again.
jwrosewell
marked this pull request as ready for review
November 11, 2026 19:33
This was referenced Nov 11, 2026
Merged
Merged
Draft
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Moves the
cxxsubmodule to 7c4ac436, the second 51degrees-cxx release of 11 November 2026, which 51degrees-cxx#150 merged intomainat 15:05 UTC, and with it reads by index on an engine built with no property names, which #101 held back until this release. The publish workflow has tagged it 4.5.105, andgit/matching-refs/tags/4.5.105gives 7c4ac436e47cd15beed304cad2b8151d50a1efd1, so the submodule is at the tagged commit.Head e24cbdf8, which is
rcat 528e0df3 merged into 905e7484. The merge brings in only #104's JSON Builder change, #49's README change and #108's CI change, none of which touches the on-premise engines, so the links below are at 905e748.cxx taken
rcbuilds against c6d4e4d9, the 10 November release, which the automated Update submodules pull request #103 moved it to. 7c4ac436 is one commit after c6d4e4d9 and brings in three 51degrees-cxx pull requests._51DEGREES_IPI_PATHin place of51DEGREES_IPI_PATH.mainback intorcwith no change to the tree.What the specification says
Finding Properties at 927ac677, merged from specifications#137, says the following.
What changed
On
rc, each on-premise engine makes its map from names to indexes from the names it was built with, and an engine built with no names has an empty map, so every read on it goes by name.DeviceDetectionHashEngineandIPIntelligenceOnPremiseEngineeach gainfixPropertyIndexes. When the engine was built with no names, it makes the map once from the native engine'sgetRequiredProperties(), and marks it made, so no later load makes it again. An engine built with names is marked made in the constructor (line 113), so its map is never replaced.fixPropertyIndexesruns first insetEngineMetaData(Device Detection line 427, IP Intelligence line 417), which every load calls. The first load runs inside the constructor throughaddDataFile(line 114), so the map is in place before the engine processes anything.getRequiredProperties()gives the name at each required index in index order (src/common/EngineBase.cpplines 119 to 137 at 7c4ac436), so a name's place in that list is its index. After the first load those are the first data file's names, which a reload keeps.getPropertyIndexand onindexOfinDeviceDataHashDefaultandIPIntelligenceDataHashDefaultnow say this.Tests
PropertyIndex_NoNamesFromFirstFile, Device Detection and IP IntelligencegetPropertyNamenames that property at that indexPropertyIndex_NoNamesSurvivesReload, Device DetectionPropertyIndex_NoNamesSurvivesReload, IP IntelligencePropertyIndex_NoNamesReadByIndexAgainstByName, Device DetectionPropertyIndex_NoNamesReadsByNamein both test classes, which required no index on such an engine, is replaced by the first row. The Device Detection names leave out the match metrics the engine adds, such asDifference, because they are not in the data file and so have no index.The cost
The engine is built with no names before and after, so processing does not change. Building adds one call to
getRequiredProperties()after the first load.A read by name against a read by index on the native results of an engine built with no names, IsMobile read 20,000 times by each in each of 5 rounds after a warm-up, in nanoseconds per read as lowest, median and highest. The engine built with names is the same test as before, for comparison.
From full build 43072290831,
PropertyIndex_NoNamesReadByIndexAgainstByNameandPropertyIndex_ReadByIndexAgainstByNamein the same job.The read by index is faster than the read by name in every job, for the engine built with no names as for the one built with names, so the index stands. The green run gave 303 ns by name against 187 ns by index with no names.
The typed read through the pipeline, before and after the change, in one job (run 43058951094). The job built 4a5c3bf (before) and 905e748 (after), then timed
getIsMobile()andget("IsMobile")50,000 times each in each of 7 rounds after a warm-up, three times for each commit in turn. Medians in nanoseconds per read.getIsMobile()beforeget("IsMobile")beforeThe engine with no names reads faster after the change in every round. The engine built with names, whose code does not change, moves by a few nanoseconds either way.
SWIG wrappers
No regeneration is needed. Both
hash_java.ifiles include onlycxx/src/common/JavaTypes.iandcxx/src/dd/hash/hash.iorcxx/src/ipi/ipi.i. The cxx.ifiles declare every class SWIG wraps, and the headers are only copied into the wrappers from%{ %}blocks.git diff cfad8978 7c4ac436 -- '*.i'andgit diff c6d4e4d9 7c4ac436 -- '*.i'in 51degrees-cxx both give one change, two lines of a//comment abovecopyValueAsUTF8Stringinsrc/ipi/ResultsIpi.ilines 115 and 116, which SWIG does not copy into a wrapper.The committed wrappers are the ones this draft generated on Ubuntu runners with SWIG 4.5.1, built from the swig/swig
v4.5.1tag, using the README commands. Withcxxat cfad8978 (run 42796095651),Java_Ipi_Engine.cppand every generated Java class of both engines came out byte for byte as committed. Withcxxat 865cee97 (run 42672898614), onlyConfigIpiSwig.javachanged, because cxx#122 scopes%newobjecttoMetaData, sogetComponents,getProperties,getValuesandgetProfilesreturn a collection the configuration owns.ConfigIpiSwig.javadiffers fromrcbecause the wrapper onrcwas last generated on 2 November (fe9d0cbd), before cxx#122, and #103 movedcxxon without generating it again.Ownership workaround removed
#69 added
borrowed()toBuilderTestsandOnPremisePerformanceProfileTests, which setswigCMemOwnto false by reflection because four getters marked their collection as owned. With the wrapper generated again, all eleven getters give a borrowed collection, so the workaround is deleted.BuilderTests.Config_CollectionsAreBorrowedreads the flag on each of the eleven collections, deletes each proxy and reads every setting again.ConfigIpiSwig.java: run 42673597556, at the ownership assertion,BuilderTests.java:123.Full build
Run 43072290831 on
tmp/pr-matrix-82b, started bytools/pr-matrix.pywithout macOS, passed. All 10Setting 'PullRequestSha'lines, in Configure and the nine build jobs, give this head, e24cbdf, and Configure printed "Left out 6 macOS entries". Every job passed, being Ubuntu Java 8, 11, 17 and 21, Ubuntu ARM Java 17, and Windows Java 8, 11, 17 and 21.The earlier full build on 905e748, run 43054665777, failed only at "Pipeline developer examples" in Ubuntu Java 17, where
SimpleCloudEngine_Testfound no resource key, and the other jobs were cancelled. #108 fixed that onrc, and this head takes it.macOS was left out of this draft's full build under the 9 November decision. The rc head is built with macOS before it merges into main.
Remaining
PropertyExcludedFromEngineConfigurationfor it.tmp/index-java-nonames-red,tmp/index-java-nonames-green,tmp/index-java-timing,tmp/pr-matrix-82andtmp/pr-matrix-82bare still to be deleted.