Use a newtyped IndexSet to back the "set" shape #1446

hlbarber · 2022-06-07T17:01:52Z

Motivation and Context

The semantics of set requires insertion ordering, however the Rust HashSet is not ordered. We are currently backing set with a std::vec::Vec, however this does not prevent duplicates.
Fixes Deduplicate repeated query string values bound with @httpQuery and @httpQueryParams to a Smithy set #972
Closes Enforce member uniqueness for set shapes #44

Description

Add a insertion ordered non-repeating collection Set<T> to aws-smithy-types
Integrate Set<T> into smithy codegen

Checklist

I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates

github-actions · 2022-06-07T17:26:01Z

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement	Deviation	Current	Old
Requests/sec	3.76%	36147.23	34835.76
Total requests	3.75%	3255191	3137532
Total errors	NaN%	0	0
Total successes	3.75%	3255191	3137532
Average latency ms	-2.13%	0.92	0.94
Minimum latency ms	0.00%	0.02	0.02
Maximum latency ms	11.03%	19.23	17.32
Stdev latency ms	7.27%	0.59	0.55
Transfer Mb	3.75%	338.38	326.15
Connect errors	NaN%	0	0
Read errors	NaN%	0	0
Write errors	NaN%	0	0
Status errors (not 2xx/3xx)	NaN%	0	0
Timeout errors	NaN%	0	0

github-actions · 2022-06-08T12:48:23Z

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement	Deviation	Current	Old
Requests/sec	15.20%	40651.54	35288.49
Total requests	15.20%	3660128	3177100
Total errors	NaN%	0	0
Total successes	15.20%	3660128	3177100
Average latency ms	-12.90%	0.81	0.93
Minimum latency ms	0.00%	0.02	0.02
Maximum latency ms	-22.81%	16.07	20.82
Stdev latency ms	-14.29%	0.48	0.56
Transfer Mb	15.20%	380.47	330.26
Connect errors	NaN%	0	0
Read errors	NaN%	0	0
Write errors	NaN%	0	0
Status errors (not 2xx/3xx)	NaN%	0	0
Timeout errors	NaN%	0	0

hlbarber · 2022-06-08T13:43:01Z

...tware/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt

@@ -864,7 +865,12 @@ private class ServerHttpBoundProtocolTraitImplGenerator(
                rust("let mut seen_${symbolProvider.toMemberName(it.member)} = false;")
            }
            queryBindingsTargettingCollection.forEach {
-                rust("let mut ${symbolProvider.toMemberName(it.member)} = Vec::new();")
+                val collection = if (model.expectShape(it.member.target) is SetShape) {


This PR introduces a ton of branches like this. Is there a way to reduce them?

yeah—you can change them to:

val collectionType = symbolProvider.toSymbol(model.expectShape(it.member.target)) val memberName = symbolProvider.toMemberName(it.member) rustTemplate("let mut $memberName = #{collection}::new();, "Collection" to collectionType)

(or similar)

hlbarber · 2022-06-08T17:08:56Z

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/rustlang/RustTypes.kt

+            const val Type = "Set"
+            const val Namespace = "aws_smithy_types"
+
+            fun RuntimeType(runtimeConfig: RuntimeConfig): RuntimeType {


We've promoted this from a val to a function in order to accommodate the need for a dependency.

hlbarber · 2022-06-08T17:10:25Z

codegen/src/test/kotlin/software/amazon/smithy/rust/lang/RustWriterTest.kt

+                    assert_eq!(test.otherMember, "hello");
+                    assert_eq!(test.member.is_empty(), true);
+                    """,
+                    "Set" to RuntimeType.Set(TestRuntimeConfig)


These tests have been "modernized" (using TestWorkspace.testProject()) in order to allow for dependency injection here.

hlbarber · 2022-06-08T17:11:22Z

codegen/src/test/kotlin/software/amazon/smithy/rust/lang/RustWriterTest.kt

+                write("member: #T,", setSymbol)
+                write("otherMember: #T,", stringSymbol)
+            }
+            it.rustBlock("fn inner()") {


We cannot do this within unitTest because it takes a string rather than provide a writer.

#1450 addresses this

Fixed b8c70b8

github-actions · 2022-06-08T17:29:17Z

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement	Deviation	Current	Old
Requests/sec	-2.96%	33948.87	34983.82
Total requests	-2.96%	3056402	3149701
Total errors	NaN%	0	0
Total successes	-2.96%	3056402	3149701
Average latency ms	3.19%	0.97	0.94
Minimum latency ms	0.00%	0.03	0.03
Maximum latency ms	30.38%	22.4	17.18
Stdev latency ms	9.43%	0.58	0.53
Transfer Mb	-2.96%	317.71	327.41
Connect errors	NaN%	0	0
Read errors	NaN%	0	0
Write errors	NaN%	0	0
Status errors (not 2xx/3xx)	NaN%	0	0
Timeout errors	NaN%	0	0

github-actions · 2022-06-08T20:13:27Z

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement	Deviation	Current	Old
Requests/sec	-9.98%	57482.38	63858.24
Total requests	-9.91%	5177971	5747689
Total errors	NaN%	0	0
Total successes	-9.91%	5177971	5747689
Average latency ms	-7.35%	0.63	0.68
Minimum latency ms	0.00%	0.02	0.02
Maximum latency ms	-8.58%	16.51	18.06
Stdev latency ms	-34.02%	0.64	0.97
Transfer Mb	-9.91%	538.25	597.48
Connect errors	NaN%	0	0
Read errors	NaN%	0	0
Write errors	NaN%	0	0
Status errors (not 2xx/3xx)	NaN%	0	0
Timeout errors	NaN%	0	0

rcoh

overall I love the direction! And thanks for modernizing some tests :-)

I think we can completely remove the differential behavior for Vec and Set with a couple of ideas I mentioned in a comment above:

Use symbolProvider.toSymbol() instead of Vec to get the type of the collection
Add a push method to Set

Thanks for addressing this extremely long standing issue!

(And hopefully someone already told you about "debug mode" codegen which makes tracking down where code is coming from much easier)

rcoh · 2022-06-09T14:01:54Z

...tware/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt

@@ -864,7 +865,12 @@ private class ServerHttpBoundProtocolTraitImplGenerator(
                rust("let mut seen_${symbolProvider.toMemberName(it.member)} = false;")
            }
            queryBindingsTargettingCollection.forEach {
-                rust("let mut ${symbolProvider.toMemberName(it.member)} = Vec::new();")
+                val collection = if (model.expectShape(it.member.target) is SetShape) {


yeah—you can change them to:

val collectionType = symbolProvider.toSymbol(model.expectShape(it.member.target)) val memberName = symbolProvider.toMemberName(it.member) rustTemplate("let mut $memberName = #{collection}::new();, "Collection" to collectionType)

(or similar)

rcoh · 2022-06-09T14:02:27Z

...tware/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt

@@ -926,7 +932,12 @@ private class ServerHttpBoundProtocolTraitImplGenerator(
                                )
                            }
                        }
-                        rust("${symbolProvider.toMemberName(it.member)}.push(v);")
+                        val method = if (model.expectShape(it.member.target) is SetShape) {
+                            "insert"


since we own Set why don't we add a push method for convenience? you can make it #[doc(hidden)] if you want

rcoh · 2022-06-09T14:03:51Z

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/rustlang/RustTypes.kt

+                return RuntimeType(name = Type, namespace = Namespace, dependency = CargoDependency.SmithyTypes(runtimeConfig))
+            }


for slightly cleaner code:
CargoDependency.SmithyTypes(runtimeConfig).asType().member("aws_smithy_types::Set")

rcoh · 2022-06-09T14:05:12Z

...ain/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/parse/JsonParserGenerator.kt

+                    var container = if (shape.isSetShape) {
+                        RuntimeType.Set(runtimeConfig)
+                    } else {
+                        RuntimeType("Vec", dependency = null, namespace = "std::vec")
+                    }


we don't need this—just use symbolProvider.toSymbol(shape)

rcoh · 2022-06-09T14:07:21Z

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/SymbolVisitor.kt

-        val builder = if (model.expectShape(shape.member.target).isStringShape) {
-            symbolBuilder(shape, RustType.HashSet(inner.rustType()))
-        } else {
-            // only strings get put into actual sets because floats are unhashable
-            symbolBuilder(shape, RustType.Vec(inner.rustType()))
-        }


let's a comment that justifies why this is OK—when this code was written, sets could contain floats but that was changed:

A set shape requires a single member named member, and the member MUST target either a string, blob, byte, short, integer, long, bigInteger, or bigDecimal shape. The targeted shape MUST NOT be marked with the streaming trait.

rcoh · 2022-06-09T14:08:16Z

rust-runtime/aws-smithy-types/src/lib.rs

 pub mod timeout;
 pub mod tristate;

 pub use crate::date_time::DateTime;
+pub use crate::set::*;


especially with pub use, lets enumerate these for clarity

…nto harryb/set-semantics

github-actions · 2022-06-10T10:37:43Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-06-10T14:22:19Z

A new generated diff is ready to view.

A new doc preview is ready to view.

rcoh

looking good so far!

rcoh · 2022-06-13T14:23:29Z

...en/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/BuilderGenerator.kt

+        docs("Appends an item to `$memberName`.")
+        rust("///")
+        docs("To override the contents of this collection use [`${member.setterName()}`](Self::${member.setterName()}).")


might as well have some custom docs that call out the deduplication

rcoh · 2022-06-13T14:24:26Z

...ain/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/parse/JsonParserGenerator.kt

+                                        if replaced {
+                                            return Err(#{JsonDeserialize}::custom("duplicate elements found while deserializing to a set"))
+                                        }


do we want to error here? or just deduplicate?

The team had a little back and forth on this (semantics =? deserialization failure cases) and opened this issue.

smithy-lang/smithy#1266

Will no longer throw an error but perhaps log a warning?

rcoh · 2022-06-13T14:26:31Z

CHANGELOG.next.toml

+references = ["smithy-rs#1446"]
+meta = { "breaking" = true, "tada" = false, "bug" = false }
+author = "hlbarber"


we'll need to call out the exact breaking change and provide upgrade instructions

github-actions · 2022-06-14T16:35:49Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Server Test (ignoring whitespace)
No codegen difference in the Server Test Python

A new doc preview is ready to view.

github-actions · 2022-06-14T16:59:45Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Server Test (ignoring whitespace)
No codegen difference in the Server Test Python

A new doc preview is ready to view.

hlbarber · 2022-08-02T17:53:54Z

Closing this due to smithy-lang/smithy#1278

Harry Barber added 3 commits June 7, 2022 14:51

Add basic Set<T> implementation

97c1f0e

Add Eq and Hash to Blob

fb76d3e

Integrate Set<T> into codegen

a8024e7

hlbarber changed the title ~~Harryb/set semantics~~ Use a newtyped IndexSet to back the smithy set shape Jun 7, 2022

hlbarber changed the title ~~Use a newtyped IndexSet to back the smithy set shape~~ Use a newtyped IndexSet to back the "set" shape Jun 7, 2022

Harry Barber added 2 commits June 7, 2022 17:05

Refactor method target shape branching

e210e2d

Add extra #[inline] to Set methods

17aef05

Harry Barber added 3 commits June 7, 2022 19:17

Reject duplicates while parsing requests

1878666

Fix type assertion in SymbolBuilder test

2b1249f

Modernize RustWriterTest tests

60b910f

Harry Barber added 3 commits June 8, 2022 13:27

Add CHANGELOG.next.toml entry

f4ae743

Refactor HashSet RustTypes

132ccc0

Remove deadcode

9a4fabf

hlbarber commented Jun 8, 2022

View reviewed changes

hlbarber marked this pull request as ready for review June 8, 2022 19:50

hlbarber requested review from a team as code owners June 8, 2022 19:50

Merge branch 'main' into harryb/set-semantics

7b346eb

rcoh requested changes Jun 9, 2022

View reviewed changes

Harry Barber and others added 5 commits June 9, 2022 20:02

Add Set::push

4728eb5

Generate Set::push rather than Set::insert

7cdf8be

Remove branches using toSymbol

b4d0fc4

Merge branch 'harryb/set-semantics' of github.com:awslabs/smithy-rs i…

89a4aa7

…nto harryb/set-semantics

Merge branch 'main' into harryb/set-semantics

eef7dd0

Add comment on the building of SetShape symbol

d73d4ee

Add protocol tests covering query and header sets

13a242a

rcoh reviewed Jun 13, 2022

View reviewed changes

Harry Barber and others added 4 commits June 14, 2022 11:13

Fix client URI assertions

99df291

Log a warning on duplication rather than reject

860c96d

Improve Set setter documentation

c6fa12b

Merge branch 'main' into harryb/set-semantics

a044069

Un-nest manually_created_struct test

b8c70b8

hlbarber closed this Aug 2, 2022

hlbarber deleted the harryb/set-semantics branch August 2, 2022 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a newtyped IndexSet to back the "set" shape #1446

Use a newtyped IndexSet to back the "set" shape #1446

hlbarber commented Jun 7, 2022 •

edited

Loading

github-actions bot commented Jun 7, 2022

github-actions bot commented Jun 8, 2022

hlbarber Jun 8, 2022

rcoh Jun 9, 2022

hlbarber Jun 8, 2022

hlbarber Jun 8, 2022

hlbarber Jun 8, 2022

hlbarber Jun 9, 2022

hlbarber Jun 14, 2022

github-actions bot commented Jun 8, 2022

github-actions bot commented Jun 8, 2022

rcoh left a comment

rcoh Jun 9, 2022

rcoh Jun 9, 2022

rcoh Jun 9, 2022

rcoh Jun 9, 2022

rcoh Jun 9, 2022

rcoh Jun 9, 2022

github-actions bot commented Jun 10, 2022

github-actions bot commented Jun 10, 2022

rcoh left a comment

rcoh Jun 13, 2022

rcoh Jun 13, 2022

hlbarber Jun 14, 2022 •

edited

Loading

hlbarber Jun 14, 2022

rcoh Jun 13, 2022

github-actions bot commented Jun 14, 2022

github-actions bot commented Jun 14, 2022

hlbarber commented Aug 2, 2022

		return RuntimeType(name = Type, namespace = Namespace, dependency = CargoDependency.SmithyTypes(runtimeConfig))
		}

Use a newtyped IndexSet to back the "set" shape #1446

Use a newtyped IndexSet to back the "set" shape #1446

Conversation

hlbarber commented Jun 7, 2022 • edited Loading

Motivation and Context

Description

Checklist

github-actions bot commented Jun 7, 2022

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

github-actions bot commented Jun 8, 2022

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

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

github-actions bot commented Jun 8, 2022

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

github-actions bot commented Jun 8, 2022

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

rcoh 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

github-actions bot commented Jun 10, 2022

github-actions bot commented Jun 10, 2022

rcoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlbarber Jun 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 14, 2022

github-actions bot commented Jun 14, 2022

hlbarber commented Aug 2, 2022

hlbarber commented Jun 7, 2022 •

edited

Loading

hlbarber Jun 14, 2022 •

edited

Loading