Skip to content
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

Jump to bottom

[Draft] ROM-based Nodestore implementation #3411

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

[Draft] ROM-based Nodestore implementation #3411

wants to merge 6 commits into from

Conversation

opcua-tsn-team-kalycito
Copy link
Contributor

  • Encode nodes present in information model
  • Dump the encoded nodes and lookup table into a file
  • Decode nodes using the lookup table after the server is in listening state
  • Added 2 CMAKE flags
    • UA_ENABLE_ENCODE_AND_DUMP - encoding and dumping of nodes
    • UA_ENABLE_USE_ENCODED_NODES - use to load the information model
  • Update encoded content during run-time
  • Added support to update string (value attribute) of nodes present in encoded binary
  • Added unit tests for encode and decode APIs

@opcua-tsn-team-kalycito opcua-tsn-team-kalycito changed the title [TASK] Binary Nodestore implementation Binary Nodestore implementation Jan 31, 2020
jpfr
jpfr requested changes Jan 31, 2020
View reviewed changes
Copy link
Member

@jpfr jpfr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Dear Kalycito Team,

wow. That is a good piece of work.
The functionality of keeping the information model in ROM where possible looks great!

Still, I have a few requests to keep the code-base well-organized.

  1. Please copy the ZipTree nodestore and make changes only in the copy.
  2. I do not want to see a single line-change under the /src directory.
    The nodestore is a plugin. The plugin API is powerful enough to keep the binary nodestore inside the plugin. There can be many different nodestores. We cannot allow the pollution from individual nodestores into the server implementation.

If you wonder how you can export (dump) the nodestore without making changes to /src: The nodestore is part of the server config. The server config is visible to the outside. You can use the iterator with the visitor pattern defined in nodestore.h to walk over all nodes for the export.

This is all public API in /include. No need to use private server API at all.

Still, this is a great effort! Just add a bit of refactoring to avoid code entropy.
Keep up the good work.

include/open62541/plugin/nodestore.h Outdated
@@ -20,6 +21,10 @@
#include <open62541/server.h>
#include "ziptree.h"

#if defined UA_ENABLE_ENCODE_AND_DUMP || defined UA_ENABLE_USE_ENCODED_NODES
Copy link
Member

Choose a reason for hiding this comment

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

The nodestore is a plugin. The server only uses the plugin API.
You can implement the binary nodestore in a plugin without changing this API.

Please, not a single line should change in nodestore.h.

Encapsulation/hiding of internals is important to keep the architecture clean.

include/open62541/server_config.h Outdated
@@ -166,6 +167,13 @@ struct UA_ServerConfig {
/* Nodestore */
UA_Nodestore nodestore;

#ifdef UA_ENABLE_USE_ENCODED_NODES
Copy link
Member

Choose a reason for hiding this comment

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

Move this inside the binary nodestore plugin.
Do not make changes in this file.

plugins/include/open62541/plugin/nodestore_default.h
@@ -2,6 +2,7 @@
* See http:https://creativecommons.org/publicdomain/zero/1.0/ for more information.
*
* Copyright 2019 (c) Julius Pfrommer, Fraunhofer IOSB
* Copyright 2020 (c) Kalycito Infotech Pvt Ltd
Copy link
Member

Choose a reason for hiding this comment

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

Remove, as no real changes were made in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jpfr Thanks for the comments.
Can we move the lookupTable structure, encode and decode function declarations to this (nodestore_default.h) file?

plugins/ua_nodestore_ziptree.c Outdated
*/

#include <open62541/plugin/nodestore_default.h>
#include <open62541/plugin/log_stdout.h>
Copy link
Member

Choose a reason for hiding this comment

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

Please make your own plugin instead of piling on top of the ZipTree Nodestore.
You can start by copying ZipTree and then making changes.

I prefer some code duplication instead of an ifdef-forest.

src/server/ua_services_attribute.c Outdated
@@ -118,7 +119,16 @@ readValueAttributeFromNode(UA_Server *server, UA_Session *session,
session->sessionHandle, &vn->nodeId,
vn->context, rangeptr, &vn->value.data.value);
UA_LOCK(server->serviceMutex);
#ifdef UA_ENABLE_USE_ENCODED_NODES
if(server->isServerListening == UA_TRUE) {
Copy link
Member

Choose a reason for hiding this comment

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

No.

This is not the place to decide whether a node is in the binary format.
This has to be done inside the nodestore plugin. It should be totally opaque to the server.

tools/nodeset_compiler/backend_open62541_typedefinitions.py Outdated
@@ -414,15 +414,19 @@ def print_encoding(self):
* on host ''' + platform.uname()[1] + ''' by user ''' + getpass.getuser() + ''' at ''' + time.strftime(
"%Y-%m-%d %I:%M:%S") + ''' */

#ifndef ''' + self.parser.outname.upper() + '''_GENERATED_ENCODING_BINARY_H_
Copy link
Member

Choose a reason for hiding this comment

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

Put changes to the nodeset compiler into a separate PR.

@jpfr jpfr changed the title Binary Nodestore implementation In-ROM Nodestore implementation Jan 31, 2020
@jpfr jpfr changed the title In-ROM Nodestore implementation ROM-based Nodestore implementation Jan 31, 2020
@opcua-tsn-team-kalycito
feat(plugin): Binary Nodestore implementation
e2bcc1f 
 - Encode nodes present in information model
 - Dump the encoded nodes and lookup table into a file
 - Decode nodes using the lookup table after the server is in
   listening state
 - Added 2 CMAKE flags for encoding and dumping of nodes and
   using it to load the information model
 - Add support update encoded content during run-time
 - Add support to update string (value attribute) into encoded binary
 - Add unit tests for encode and decode APIs

Change-Id: I11426b0d51b3d79c3d1d2739d5ef90cd0526dd7e
Signed-off-by: Jayanth Velusamy <[email protected]>
jpfr
jpfr reviewed Feb 11, 2020
View reviewed changes
Copy link
Member

@jpfr jpfr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Really good!
This iteration is a nice improvement. Thanks for putting in the work.
A few more comments remain. But this looks good from an overall architecture point of view.

plugins/ua_nodestore_binary_encoded.c Outdated
#define OFFSET_MEMORYSIZE 2024 // Offset memory size for the edited encode nodes
#define MAX_ROW_LENGTH 30 // Maximum length of a row in lookup table

lookUpTable *ltRead;
Copy link
Member

Choose a reason for hiding this comment

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

Global variables should be avoided in the library.
A user might instantiate more than one server in the same process.

plugins/ua_nodestore_binary_encoded.c Outdated
static void
readRowLookuptable(char ltRow [], int length, lookUpTable *lt, int row) {
/**
* There are four contents per row
Copy link
Member

Choose a reason for hiding this comment

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

There is a standard text format for NodeIds.
Like i=13, ns=10;s=Hello:World, g=09087e75-8e5e-499b-954f-f2a9603db28a, ns=1;b=M/RbKBsRVkePCePcx24oRA==, etc.
See Part 6, Section 5.3.1.10. Let's try to write and parse that.

So we are known to be complete.

Base64 encoding/decoding is included in the dependencies.
And already used for the JSON parser.

Here's a snippet to parse guids of the form 00000000-0000-0000-0000-000000000000 from a char-array arg:

    UA_Guid *guid = UA_Guid_new();
    guid->data1 = strtoull(arg, NULL, 16);
    guid->data2 = strtoull(&arg[9], NULL, 16);
    guid->data3 = strtoull(&arg[14], NULL, 16);
    UA_Int16 data4_1 = strtoull(&arg[19], NULL, 16);
    guid->data4[0] = data4_1 >> 8;
    guid->data4[1] = data4_1;
    UA_Int64 data4_2 = strtoull(&arg[24], NULL, 16);
    guid->data4[2] = data4_2 >> 40;
    guid->data4[3] = data4_2 >> 32;
    guid->data4[4] = data4_2 >> 24;
    guid->data4[5] = data4_2 >> 16;
    guid->data4[6] = data4_2 >> 8;
    guid->data4[7] = data4_2;

Copy link
Member

Choose a reason for hiding this comment

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

Here's an extended snippet for NodeId parsing I still had from an older project.
You can quote form that.

/* Parse NodeId from String */
static UA_StatusCode
parseNodeId(UA_String str, UA_NodeId *id) {
                /* zero-terminated string */
    UA_STACKARRAY(char, s, str.length + 1);
                memcpy(s, str.data, str.length);
                s[str.length] = 0;

                UA_NodeId_init(id);

                /* Int identifier */
                if(sscanf(s, "ns=%hu;i=%u", &id->namespaceIndex, &id->identifier.numeric) == 2 ||
       sscanf(s, "ns=%hu,i=%u", &id->namespaceIndex, &id->identifier.numeric) == 2 ||
       sscanf(s, "i=%u", &id->identifier.numeric) == 1) {
                               id->identifierType = UA_NODEIDTYPE_NUMERIC;
                               return UA_STATUSCODE_GOOD;
                }

    /* String identifier */
    UA_STACKARRAY(char, sid, str.length);
                if(sscanf(s, "ns=%hu;s=%[^\n]", &id->namespaceIndex, sid) == 2 ||
       sscanf(s, "ns=%hu,s=%[^\n]", &id->namespaceIndex, sid) == 2 ||
       sscanf(s, "s=%s", sid) == 1) {
                               id->identifierType = UA_NODEIDTYPE_STRING;
                               id->identifier.string = UA_String_fromChars(sid);
                               return UA_STATUSCODE_GOOD;
    }

    /* ByteString idenifier */
                if(sscanf(s, "ns=%hu;b=%[^\n]", &id->namespaceIndex, sid) == 2 ||
       sscanf(s, "ns=%hu,b=%[^\n]", &id->namespaceIndex, sid) == 2 ||
       sscanf(s, "b=%s", sid) == 1) {
                               id->identifierType = UA_NODEIDTYPE_BYTESTRING;
                               id->identifier.string = UA_String_fromChars(sid);
                               return UA_STATUSCODE_GOOD;
    }

    /* Guid idenifier */
                if(sscanf(s, "ns=%hu;g=%s", &id->namespaceIndex, sid) == 2 ||
       sscanf(s, "ns=%hu,g=%s", &id->namespaceIndex, sid) == 2 ||
       sscanf(s, "g=%s", sid) == 1) {
        UA_StatusCode retval = parseGuid(sid, &id->identifier.guid);
        if(retval != UA_STATUSCODE_GOOD)
            return retval;
                               id->identifierType = UA_NODEIDTYPE_GUID;
                               return UA_STATUSCODE_GOOD;
    }

                return UA_STATUSCODE_BADINTERNALERROR;
}

plugins/ua_nodestore_binary_encoded.c Outdated
}
fclose(fpLookuptable);
*ltSize = nodeCount;
lookUpTable *lt = (lookUpTable *)UA_calloc(*ltSize, sizeof(lookUpTable));
Copy link
Member

Choose a reason for hiding this comment

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

Check for NULL every time memory is allocated.
And handle the error of course. We are in memory constrained environments.

This method might be simpler if you just pass in the pointer to lookupTable.

plugins/ua_nodestore_binary_encoded.c

NodeEntry *entry = container_of(node, NodeEntry, nodeId);
++entry->refCount;
entry->deleted = true; // remove the decoded node from the memory when release is called!
Copy link
Member

Choose a reason for hiding this comment

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

You could just remove the deleted member of the entries.
I think you always want to delete when the refocunt is zero.
So you don't need that at all.

plugins/ua_nodestore_binary_encoded.c
size_t referencesSize;
retval |= UA_UInt64_decodeBinary(&encodedBin, &offset, (UA_UInt64 *)&referencesSize);

UA_Node* node = zipNsNewNode(ctx, nodeClass);
Copy link
Member

Choose a reason for hiding this comment

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

You need to handle the case where malloc in zipNsNewNode fails.

plugins/ua_nodestore_binary_encoded.c Outdated
UA_Node*
decodeNode(void *ctx, UA_ByteString encodedBin, size_t offset) {
UA_StatusCode retval = UA_STATUSCODE_GOOD;
UA_NodeId nodeID;
Copy link
Member

Choose a reason for hiding this comment

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

If you malloc the new node first, then you can directly decode "into" the node.
Instead of allocating on the stack and then doing a memcpy.

plugins/ua_nodestore_binary_encoded.c Outdated
ZipContext *ns = (ZipContext*)nsCtx;
NodeEntry dummy;
if(isMinimalNodesAdded) {
if(nodeId->identifierType == UA_NODEIDTYPE_NUMERIC) {
Copy link
Member

Choose a reason for hiding this comment

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

You can just use UA_NodeId_order to test for equality.
Then you don't have to make a difference between string and numeric here.

Also, if you order the Nodes in the lookuptable (with UA_NodeId_order) after parsing from the file, then you can get O(log n) lookup speed by doing binary search.
Instead of the linear lookup speed you have here.

plugins/ua_nodestore_binary_encoded.c Outdated
#endif

#ifdef UA_ENABLE_USE_ENCODED_NODES
#define MINIMALNODECOUNT 45 // Total number of nodes in MINIMAL configuration
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why these 45 nodes get special treatment.
Can you add a bit of documentation?

I'd prefer (by far!) if we could handle all nodes the same for the binary nodestore.
Maybe we can just rip out the ns0 initialization in ua_server_ns0 if the binary nodestore is provided.
So we can assume they are "pre-initialized" in the binary nodestore.

Copy link
Member

Choose a reason for hiding this comment

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

By the way, this contradicts my previous position that no changes in /src are permitted. 😄
But changing one line (init_ns0) is less of a problem than tight integration.

@jpfr
fix(plugin): ROMmable nodestore only with immutable nodes (copy on wr…
97e09ac 
…ite)
@jpfr
refactor(plugin): mmap the binary nodestore file to save RAM
306e40a
@jpfr
refactor(plugin): Don't overwrite the ROM nodestore at runtime
bb67d65
@jpfr
test: Disable ns0 initialization to save RAM
154d504 
Memory measurements with

	valgrind --tool=massif ./server

show the following improvements for the minmal nodeset:

Before

    KB
49.41^                                                                     #
     |                                                                @    #
     |                                                         @  @:@ @::::#:
     |                                                     @ @ @::@:@:@::::#:
     |                                              :  @ @:@:@:@::@:@:@::::#:
     |                                        @ @ :::::@:@:@:@:@::@:@:@::::#:
     |                                   @    @:@::::: @ @:@:@:@::@:@:@::::#:
     |                              : @  @::::@:@::::: @ @:@:@:@::@:@:@::::#:
     |                        @@ : :::@::@ :::@:@::::: @ @:@:@:@::@:@:@::::#:
     |                     @::@ ::::: @: @ :::@:@::::: @ @:@:@:@::@:@:@::::#:
     |                  :::@::@ ::::: @: @ :::@:@::::: @ @:@:@:@::@:@:@::::#::
     |                :::::@::@ ::::: @: @ :::@:@::::: @ @:@:@:@::@:@:@::::#::
     |            ::::: :::@::@ ::::: @: @ :::@:@::::: @ @:@:@:@::@:@:@::::#::
     |           :::::: :::@::@ ::::: @: @ :::@:@::::: @ @:@:@:@::@:@:@::::#::
     |         @::::::: :::@::@ ::::: @: @ :::@:@::::: @ @:@:@:@::@:@:@::::#::
     |         @::::::: :::@::@ ::::: @: @ :::@:@::::: @ @:@:@:@::@:@:@::::#::
     |        @@::::::: :::@::@ ::::: @: @ :::@:@::::: @ @:@:@:@::@:@:@::::#::
     |        @@::::::: :::@::@ ::::: @: @ :::@:@::::: @ @:@:@:@::@:@:@::::#::
     |     ::@@@::::::: :::@::@ ::::: @: @ :::@:@::::: @ @:@:@:@::@:@:@::::#::
     |     : @@@::::::: :::@::@ ::::: @: @ :::@:@::::: @ @:@:@:@::@:@:@::::#::
   0 +----------------------------------------------------------------------->Mi
     0                                                                   3.926

After

    KB
18.03^                                                         ::#
     |                                                         : #
     |                                                         : #
     |                                                         : #
     |                                                         : #:::::::@::
     |                                                       ::: # ::    @::
     |                                                 @    :@:: # ::    @::
     |                                                 @   ::@:: # ::    @::
     |                                                 @   ::@:: # ::    @::
     |                                                 @  @::@:: # ::    @::
     |                                                 @::@::@:: # ::    @:::
     |                                                 @: @::@:: # ::    @::@
     |                                                :@: @::@:: # ::    @::@
     |                                  :::::::::::::@:@: @::@:: # ::    @::@
     |                                  :            @:@: @::@:: # ::    @::@@
     |                               :@::            @:@: @::@:: # ::    @::@@
     |                               :@ :            @:@: @::@:: # ::    @::@@
     |                               :@ :            @:@: @::@:: # ::    @::@@
     |                               :@ :            @:@: @::@:: # ::    @::@@
     |                               :@ :            @:@: @::@:: # ::    @::@@
   0 +----------------------------------------------------------------------->ki
     0                                                                   623.1
@lgtm-com
Copy link

lgtm-com bot commented Feb 12, 2020

This pull request introduces 2 alerts when merging 154d504 into 74eab7d - view on LGTM.com

new alerts:

  • 2 for Implicit function declaration

@jpfr jpfr mentioned this pull request Feb 13, 2020
Closed
@opcua-tsn-team-kalycito
refactor(plugin): Multiple fixes
75421d1 
 - Remove Global variable usage
 - Add binary search for lookUpTable contents
 - Add CMAKE flag for disabling init_ns0()
 - Remove memcpy in decodeNode()

Signed-off-by: Jayanth Velusamy <[email protected]>
@Stasik0
Copy link
Contributor

Stasik0 commented Oct 28, 2020

bump

@jonasgreen88
Copy link
Contributor

Hi @opcua-tsn-team-kalycito, Looking forward to this PR. What is the schedule for this PR? What is left to complete it?

@jpfr jpfr mentioned this pull request Apr 7, 2021
Closed
7 tasks
@jpfr jpfr mentioned this pull request Apr 22, 2022
Open
33 tasks
@andreasebner andreasebner marked this pull request as draft May 10, 2022 14:15
@andreasebner andreasebner changed the title ROM-based Nodestore implementation [Draft] ROM-based Nodestore implementation May 10, 2022
@Rehcsif
Copy link

Rehcsif commented Mar 21, 2023

I also try to work with open62541 on embedded devices. This PR is therefore very interesting. What is missing in the ROM-based Nodestore implementation, is there any timeline/attempt to merge this back to main?

@jpfr
Copy link
Member

jpfr commented Mar 22, 2023

A few people are interested in this.
But this won’t be on master until ca. autumn 2023.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpfr jpfr Awaiting requested review from jpfr

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@opcua-tsn-team-kalycito @Stasik0 @jonasgreen88 @Rehcsif @jpfr