Added sink type toString to the serialization #2493

ferencdg · 2020-11-12T08:15:38Z

This change should reduce the number of memory allocations when using toString custom serialization

ferencdg · 2020-11-12T08:26:04Z

data/vibe/data/serialization.d

@@ -475,7 +475,10 @@ private template serializeValueImpl(Serializer, alias Policy) {
 			ser.serializeValue!(CustomType, ATTRIBUTES)(value.toRepresentation());
 		} else static if (isISOExtStringSerializable!TU) {
 			ser.serializeValue!(string, ATTRIBUTES)(value.toISOExtString());
-		} else static if (isStringSerializable!TU) {
+		} else static if (ObjectSupportSinkType!TU && SerializerSupportSinkType!(Serializer, TU)) {


ObjectSupportSinkType is not technically needed as SerializerSupportSinkType implicitly checks for the same

ferencdg · 2020-11-12T08:27:05Z

data/vibe/data/json.d

+			dg = (scope const(char)[] data)
+			{
+				static if (__traits(compiles, m_range ~= data))
+					m_range ~= data;


in case the OutputRange supports more than just the put method(like with Appender) and operator~ is more optimized

It would be better to have this as a comment in the code ;)

Geod24

The duplication between serializer and object handling looks a bit odd to me, but I'm not familiar enough with the code to suggest an alternative (or be 100% sure there's a better way either). So LGTM, minus a few comments. @s-ludwig ?

Geod24 · 2020-11-16T16:13:17Z

data/vibe/data/json.d

+			dg = (scope const(char)[] data)
+			{
+				static if (__traits(compiles, m_range ~= data))
+					m_range ~= data;


It would be better to have this as a comment in the code ;)

Geod24 · 2020-11-16T16:21:30Z

data/vibe/data/serialization.d

+	import std.format : formattedWrite;
+	import vibe.data.json;
+
+	struct X (alias T)


Suggested change

struct X (alias T)

struct X (bool hasSink)

Would be more descriptive, wouldn't it ?

Geod24 · 2020-11-16T16:25:55Z

data/vibe/data/json.d

+		static if (isOutputRange!(R, char))
+		{
+			dg = (scope const(char)[] data)
+			{
+				static if (__traits(compiles, m_range ~= data))
+					m_range ~= data;
+				else
+					foreach (const c; data)
+						m_range.put(c);
+			};
+		}


Having this delegate in the ctor has the downside of creating an extra GC allocation and an extra reference to m_range through the context.
It would be much better to move this delegate to the function that pass it (serializeSinkType) and use scope.
But even better, you could just make it a private method and pass &this.sink at L2028 instead of dg.

Geod24 · 2020-11-16T16:30:26Z

data/vibe/data/serialization.d

+}
+
+///
+package template isObjectSupportSinkType (ObjectT)


Suggested change

package template isObjectSupportSinkType (ObjectT)

package template isSinkSerializable (ObjectT)

I think the name is shorter and still fits with the existing style, what do you think ?

Geod24 · 2020-11-16T16:32:20Z

data/vibe/data/serialization.d

+///
+package template isObjectSupportSinkType (ObjectT)
+{
+	enum isObjectSupportSinkType = is(typeof(ObjectT.toString(cast(SerializeSinkDg) null)) == void) &&


Suggested change

enum isObjectSupportSinkType = is(typeof(ObjectT.toString(cast(SerializeSinkDg) null)) == void) &&

enum isObjectSupportSinkType = is(typeof(ObjectT.toString(SerializeSinkDg.init))) &&

X.init is a neat way to get a default value typed as X. Remember, every cast is an aberration ;)
And I don't think we need to force the return type to be void. Why would we ?

Geod24 · 2020-11-16T16:32:40Z

data/vibe/data/serialization.d

+		private string s;
+		this (int i, string s) @safe {this.i = i; this.s = s;}
+
+		static if(T)


Geod24 · 2020-11-16T16:33:07Z

data/vibe/data/serialization.d

+	{
+		private int i;
+		private string s;
+		this (int i, string s) @safe {this.i = i; this.s = s;}


If it's a struct, you don't need the ctor, do you ?
If you do, missing attributes.

Geod24 · 2020-11-16T16:33:16Z

data/vibe/data/serialization.d

+			}
+		}
+
+		public string toString () @safe


Suggested change

public string toString () @safe

public string toString () const @safe

Geod24 · 2020-11-16T16:35:33Z

data/vibe/data/serialization.d

+///
+package template isSerializerSupportSinkType (SerT, ObjT)
+{
+	enum isSerializerSupportSinkType = is(typeof(SerT.serializeSinkType!(ObjT)(ObjT.init)) == void);


Same comment about void return as below

ferencdg · 2020-11-17T08:14:16Z

The duplication between serializer and object handling looks a bit odd to me, but I'm not familiar enough with the code to suggest an alternative (or be 100% sure there's a better way either). So LGTM, minus a few comments. @s-ludwig ?

we have serializers like JsonSerializer(differenet from JsonStringSerializer) and JsonSerializer writes into a Json temporary object instead of an OutputRange. Because of this my thinking was that this sink serialization has to be supported by both the object and the serializer

ferencdg · 2020-11-17T08:15:38Z

thans for the comments @Geod24, I fixed them(same for #2492)

This change should reduce the number of memory allocations when using toString custom serialization

s-ludwig · 2020-11-26T19:02:01Z

data/vibe/data/json.d

+		{
+			static if (isOutputRange!(R, char))
+			{
+				static if (__traits(compiles, m_range ~= data))


This needs to perform jsonEscape, or is the idea is that the object is coupled with the output serialization format and can write arbitrary data?

ferencdg commented Nov 12, 2020

View reviewed changes

ferencdg force-pushed the support-sink-serialization branch from 549f6a0 to acff461 Compare November 12, 2020 11:35

ferencdg mentioned this pull request Nov 16, 2020

Implemented support for custom serialization #2492

Merged

Geod24 approved these changes Nov 16, 2020

View reviewed changes

ferencdg force-pushed the support-sink-serialization branch from 7adf705 to 80f1bb9 Compare November 17, 2020 08:10

Added sink type toString to the serialization

39a280f

This change should reduce the number of memory allocations when using toString custom serialization

ferencdg force-pushed the support-sink-serialization branch from 69fff52 to 39a280f Compare November 20, 2020 08:58

Geod24 approved these changes Nov 23, 2020

View reviewed changes

Geod24 merged commit d5d47d1 into vibe-d:master Nov 23, 2020

Geod24 mentioned this pull request Nov 24, 2020

[upstream] Implement support for toString(sink) in Vibe.d bosagora/agora#557

Closed

s-ludwig mentioned this pull request Nov 25, 2020

Sink serialization: Do not provide the type explicitly #2498

Merged

s-ludwig reviewed Nov 26, 2020

View reviewed changes

s-ludwig mentioned this pull request Nov 26, 2020

Serialization rework #2503

Merged

Added sink type toString to the serialization #2493

Added sink type toString to the serialization #2493

ferencdg commented Nov 12, 2020

ferencdg Nov 12, 2020

ferencdg Nov 12, 2020

Geod24 Nov 16, 2020

Geod24 left a comment

Geod24 Nov 16, 2020

Geod24 Nov 16, 2020

Geod24 Nov 16, 2020

Geod24 Nov 16, 2020

Geod24 Nov 16, 2020

Geod24 Nov 16, 2020

Geod24 Nov 16, 2020

Geod24 Nov 16, 2020

Geod24 Nov 16, 2020

ferencdg commented Nov 17, 2020

ferencdg commented Nov 17, 2020

s-ludwig Nov 26, 2020

	package template isObjectSupportSinkType (ObjectT)
	package template isSinkSerializable (ObjectT)

	enum isObjectSupportSinkType = is(typeof(ObjectT.toString(cast(SerializeSinkDg) null)) == void) &&
	enum isObjectSupportSinkType = is(typeof(ObjectT.toString(SerializeSinkDg.init))) &&

	public string toString () @safe
	public string toString () const @safe

Added sink type toString to the serialization #2493

Added sink type toString to the serialization #2493

Conversation

ferencdg commented Nov 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Geod24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferencdg commented Nov 17, 2020

ferencdg commented Nov 17, 2020

Choose a reason for hiding this comment