NodesetCompiler use Byte array instead of string #1621

nizamogluyekta · 2018-02-28T13:57:21Z

Due to issue #1254 : The huge strings are changed as arrays of ASCII values of each element inside for ByteArrays. And also some implementations are done due to task.

Fixes #1254

Pro

Thanks! As discussed in person earlier, here some comments:

Now that we do not have the problem of too long strings, you can completely remove the max_string_length parameter
There is still some memory problem. See here:
https://travis-ci.org/open62541/open62541/jobs/347280202#L6644

In general I think it would be better to try to avoid the additional malloc call since the data is already on the stack we can just pass it as a reference

Pro · 2018-02-28T16:08:52Z

tools/nodeset_compiler/backend_open62541_datatypes.py

@@ -40,7 +46,15 @@ def generateXmlElementCode(value, alloc=False, max_string_length=0):
    return "UA_XMLELEMENT{}({})".format("_ALLOC" if alloc else "", splitStringLiterals(value, max_string_length=max_string_length))

 def generateByteStringCode(value, alloc=False, max_string_length=0):
-    return "UA_BYTESTRING{}({})".format("_ALLOC" if alloc else "", splitStringLiterals(value, max_string_length=max_string_length))
+   return "char stringArr?{}! = {};\n\


Why do you use this cryptic ? and ! notation and replace it later? Can't you directly use []? Adding the byte array then has to use the correct brackets.

Use 4 spaces indentation

The alloc parameter is not used anymore, remove it from the parameters

Pro · 2018-02-28T16:09:30Z

tools/nodeset_compiler/backend_open62541_datatypes.py

+    variable.data = (UA_Byte *)UA_malloc(variable.length);\n\
+    memcpy(&stringArr, &variable.data, variable.length)".format(len(stringtoByteArray(value, max_string_length=max_string_length)) \
+                                                                ,stringtoByteArray(value, max_string_length=max_string_length),\
+                                                                len(stringtoByteArray(value, max_string_length=max_string_length)) )


Try to avoid calling stringtoByteArray three times. Just store the result before and then use the resulting value here.

Pro · 2018-02-28T16:10:10Z

tools/nodeset_compiler/backend_open62541_datatypes.py

-        return "UA_BYTESTRING_NULL" if not node.value else generateByteStringCode(re.sub(r">\s*<", "><", re.sub(r"[\r\n]+", "", node.value)), alloc=asIndirect, max_string_length=max_string_length)
+        return prepend + "UA_BYTESTRING_NULL" if not node.value else generateByteStringCode(re.sub(r">\s*<", "><", re.sub(r"[\r\n]+", "", node.value)),
+                                      alloc=asIndirect, max_string_length=max_string_length).replace("[","{").replace("]","}")\
+                                                                                            .replace("?","[").replace("!","]")


See previous comment, then remove the last two replace operations.

Pro · 2018-02-28T16:14:47Z

tools/nodeset_compiler/backend_open62541_nodes.py

@@ -521,7 +521,7 @@ def generateNodeCode_begin(node, nodeset, max_string_length, generate_ns0, paren
        code.append("UA_NODEID_NULL,")
    code.append("(const UA_NodeAttributes*)&attr, &UA_TYPES[UA_TYPES_{}ATTRIBUTES],NULL, NULL);".format(node.__class__.__name__.upper().replace("NODE" ,"")))
    code.extend(codeCleanup)
-    
+


Try to avoid changes like this

Pro · 2018-02-28T16:16:44Z

tools/nodeset_compiler/backend_open62541_datatypes.py

+   return "char stringArr?{}! = {};\n\
+    UA_ByteString variable;\n\
+    variable.length = {};\n\
+    variable.data = (UA_Byte *)UA_malloc(variable.length);\n\


Add a check if variable.data is NULL and return UA_STATUSCODE_BADOUTOFMEMORY.

coveralls · 2018-03-07T10:23:45Z

Coverage decreased (-0.08%) to 77.076% when pulling 81d76ca on nizamogluyekta:feature/nodeset_bytearray into 961f0db on open62541:master.

Pro · 2018-07-11T11:28:01Z

@jpfr the Opc.Ua node is a huge bytestring. With this PR we improve the handling of huge bytestrings, but currently there is the following issue:

The generated code looks like this:

/* Opc.Ua - ns=0;i=7617 */

static UA_StatusCode function_ua_namespace0_146_begin(UA_Server *server, UA_UInt16* ns) {
UA_StatusCode retVal = UA_STATUSCODE_GOOD;
UA_VariableAttributes attr = UA_VariableAttributes_default;
attr.minimumSamplingInterval = 0.000000;
attr.userAccessLevel = 1;
attr.accessLevel = 1;
attr.valueRank = -1;
attr.dataType = UA_NODEID_NUMERIC(ns[0], 15);
UA_ByteString *variablenode_ns_0_i_7617_variant_DataContents =  UA_ByteString_new();
UA_Byte stringArr[12777] = {60, 111, 112, 99, /* ... */ , 114, 121, 62};
variablenode_ns_0_i_7617_variant_DataContents->length = 12777;
variablenode_ns_0_i_7617_variant_DataContents->data = stringArr;
UA_Variant_setScalar(&attr.value, variablenode_ns_0_i_7617_variant_DataContents, &UA_TYPES[UA_TYPES_BYTESTRING]);
attr.displayName = UA_LOCALIZEDTEXT("", "Opc.Ua");
attr.description = UA_LOCALIZEDTEXT("", "");
attr.writeMask = 0;
attr.userWriteMask = 0;
retVal |= UA_Server_addNode_begin(server, UA_NODECLASS_VARIABLE,
UA_NODEID_NUMERIC(ns[0], 7617),
UA_NODEID_NUMERIC(ns[0], 93),
UA_NODEID_NUMERIC(ns[0], 47),
UA_QUALIFIEDNAME(ns[0], "Opc.Ua"),
UA_NODEID_NUMERIC(ns[0], 72),
(const UA_NodeAttributes*)&attr, &UA_TYPES[UA_TYPES_VARIABLEATTRIBUTES],NULL, NULL);
UA_ByteString_delete(variablenode_ns_0_i_7617_variant_DataContents);
return retVal;
}

As you can see stringArr contains the binary data for Opc.Ua which is stored on the stack. UA_Server_addNode_begin will add the node and copy the variable attributes, i.e. it will alloc a new heap variable and copy over the 12kB. On embedded platforms we then have the same huge amount of data two times (stack+heap).

Do you have a good idea, how we can avoid the allocation and just take a reference to the data which is already there maybe in a global variable?
I thought about adding an additional UA_VariantStorageType like UA_VARIANT_DATA_NOCOPY where UA_Variant_copy takes the pointer instead of copying it. But to do that we need to change the UA_Variant_copy method, which is generated by generate_datatypes.py. Is there a better way to handle this?

jpfr · 2018-07-12T19:15:31Z

I thought about adding an additional UA_VariantStorageType like UA_VARIANT_DATA_NOCOPY where UA_Variant_copy takes the pointer instead of copying it.

Is the additional complexity worth it?

If we need absolute minimum resource consumption, we could implement the binary nodeset file format. We could have a special nodestore that uses zero initialization and generates node only when they are used.

http://documentation.unified-automation.com/uasdkhp/1.1.1/html/md_opcua_binary_fileformat.html

Rumor has the binary nodeset format will play an important role in the standard.

Pro · 2018-07-16T07:18:39Z

Ok, then I will try to get CI green again with the current status, and skip the issue of double memory usage for the (huge) nodes.

fixes open62541#1254 The huge strings are changed as arrays of ASCII values of each element inside for ByteArrays. And also some implementations are done due to task.

lock · 2019-07-31T15:00:45Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Pro requested changes Feb 28, 2018

View reviewed changes

Pro changed the title ~~solution for https://github.com/open62541/open62541/issues/1254~~ NodesetCompiler use Byte array instead of string Mar 2, 2018

Pro added this to the 0.4 milestone Mar 2, 2018

Pro approved these changes Mar 21, 2018

View reviewed changes

Pro force-pushed the feature/nodeset_bytearray branch from d2d20fd to 81d76ca Compare March 21, 2018 14:24

Pro force-pushed the feature/nodeset_bytearray branch from 81d76ca to eb48fdc Compare July 11, 2018 11:19

Pro self-assigned this Jul 16, 2018

Use byte array instead of string

c47cef6

fixes open62541#1254 The huge strings are changed as arrays of ASCII values of each element inside for ByteArrays. And also some implementations are done due to task.

Pro force-pushed the feature/nodeset_bytearray branch 2 times, most recently from 16d8225 to 5f3b475 Compare July 16, 2018 08:28

Pro added 2 commits July 24, 2018 09:30

Remove max-string-length parameter for nodeset compiler

ee63846

Flush nodeset generated file to avoid buffer race conditions

30c2c5b

Pro force-pushed the feature/nodeset_bytearray branch from ab9e06e to 30c2c5b Compare July 24, 2018 07:31

Use global variables for byte array to improve performance

6bd48c8

Pro force-pushed the feature/nodeset_bytearray branch from d9e4c13 to 6bd48c8 Compare July 31, 2018 12:14

Pro merged commit 8158f37 into open62541:master Jul 31, 2018

Pro mentioned this pull request Sep 2, 2018

Feature Request: Add huge string arrays by referencing ROM #2036

Open

lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NodesetCompiler use Byte array instead of string #1621

NodesetCompiler use Byte array instead of string #1621

nizamogluyekta commented Feb 28, 2018 •

edited by Pro

Pro left a comment

Pro Feb 28, 2018

Pro Feb 28, 2018

Pro Feb 28, 2018

Pro Feb 28, 2018

Pro Feb 28, 2018

Pro Feb 28, 2018

Pro Feb 28, 2018

coveralls commented Mar 7, 2018 •

edited

Pro commented Jul 11, 2018

jpfr commented Jul 12, 2018

Pro commented Jul 16, 2018

lock bot commented Jul 31, 2019

NodesetCompiler use Byte array instead of string #1621

NodesetCompiler use Byte array instead of string #1621

Conversation

nizamogluyekta commented Feb 28, 2018 • edited by Pro

Pro left a comment

Choose a reason for hiding this comment

Pro Feb 28, 2018

Choose a reason for hiding this comment

Pro Feb 28, 2018

Choose a reason for hiding this comment

Pro Feb 28, 2018

Choose a reason for hiding this comment

Pro Feb 28, 2018

Choose a reason for hiding this comment

Pro Feb 28, 2018

Choose a reason for hiding this comment

Pro Feb 28, 2018

Choose a reason for hiding this comment

Pro Feb 28, 2018

Choose a reason for hiding this comment

coveralls commented Mar 7, 2018 • edited

Pro commented Jul 11, 2018

jpfr commented Jul 12, 2018

Pro commented Jul 16, 2018

lock bot commented Jul 31, 2019

nizamogluyekta commented Feb 28, 2018 •

edited by Pro

coveralls commented Mar 7, 2018 •

edited