Add functions for reading and writing length-prefixed data with customizable encodings for the length #2867

hamzahrmalik · 2024-09-04T18:38:13Z

Add functions for reading and writing length-prefixed data with customizable encodings for the length, particularly for quic variable-length integers (RFC 9000)

Motivation:

Many protocols require us to write data and then prefix that data with its length. But each protocol has a different way of encoding the length. This PR introduces general purpose functions which can be extended for different encoding strategies

Modifications:

Create a new protocol which defines how to encode an integer
Implement this protocol for QUIC

Provide functions on bytebuffer for writing length-prefixed buffers, strings or bytes

…misable encodings for the length

glbrntt

This is neat! I've not reviewed it all but left some initial feedback and questions to make sure I understood how it all fits together.

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

glbrntt · 2024-09-05T08:00:13Z

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

+    /// If you use write more or less than this many bytes, we wll need to move bytes around to make that possible
+    /// Therefore, you may decide to encode differently to use more bytes than you actually would need, if it is possible for you to do so
+    /// That way, you waste the reserved bytes, but improve writing performance
+    func writeIntegerWithReservedSpace(


This API 'stutters' a little bit: could we use reservedCapacity to overload writeInteger? i.e.

func writeInteger(_ value: Int, to buffer: inout ByteBuffer, reservedCapacity: Int) -> Int

glbrntt · 2024-09-05T08:11:27Z

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

+        to buffer: inout ByteBuffer
+    ) -> Int
+
+    /// When writing an integer using this strategy, how many bytes we should reserve


I found this and writeIntegerWithReservedSpace quite unintuitive without looking at how they were used. It became clearer when I started reading "Integer" as "Length". Is that the right way to think about it?

Yeah i agree this is confusing

The idea here is that reservedSpaceForInteger is the amount of space that someone should reserve if theyre intending to write an integer using your strategy
And the writeIntegerWithReservedSpace is for when someone has already reserved some space, and now wants you to write an integer into that space. You can then make the decision on whether you want to try and use exactly that much space (this is what the quic impl does) even if you could've used less. Ie you choose whether you optimize for the smallest possible encoding of the int, or for trying to avoid moving bytes around

This is in practice currently only used by the writePrefixed* functions so yes, there the integer is a length. But i don't think it has to be a length

It can be used to write an integer, but in the context of this protocol should it be used for anything other than the length?

IMO we should make the intent of the requirement clear, if you wanted to write an integer then you should just call the writeInteger(_:to:) method.

Sources/NIOCore/ByteBuffer-quicBinaryEncodingStrategy.swift

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

glbrntt

Left a few more comments. The bulk of this looks great but still unsure around some of the naming.

glbrntt · 2024-09-10T09:34:56Z

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

+    /// If you use write more or less than this many bytes, we wll need to move bytes around to make that possible.
+    /// Therefore, you may decide to encode differently to use more bytes than you actually would need, if it is possible for you to do so.
+    /// That way, you waste the reserved bytes, but improve writing performance.
+    func writeIntegerWithReservedCapacity(


nit: repeating "reserved" in the name and in the parameter name. Is that necessary?

George's note here is good, we can probably remove the note in the name.

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

weissi · 2024-09-12T14:28:54Z

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

+#elseif canImport(Bionic)
+import Bionic
+#else
+#error("The Byte Buffer module was unable to identify your C library.")


@MaxDesiatov don't we need WASIlibc here?

Yes, thanks for catching this!

If we need this, then it's missing across all of NIO

weissi · 2024-09-12T14:29:41Z

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

+//===----------------------------------------------------------------------===//
+
+#if os(Windows)
+import ucrt


@hamzahrmalik why do we need the libcs here?

for the memmove function

Okay, in this case we should remove the imports here.

we shouldn't use *Unsafe* here, the most minimal subset of ByteBuffer-core.swift needs to provide enough

memmove isn't necessary to be called directly. UnsafeRawBufferPointer can move memory with a higher-level API (I think it's bufferPointer[0..<100] = bufferPointer[2..<102] or something like that; but again, no Unsafe in here please)

MaxDesiatov · 2024-09-12T14:30:27Z

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

+#elseif canImport(Musl)
+import Musl
+#elseif canImport(Bionic)
+import Bionic


Suggested change

import Bionic

import Bionic

#elseif canImport(WASILibc)

import WASILibc

weissi · 2024-09-12T14:30:57Z

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

+        // Add the required number of bytes to the end first
+        self.writeRepeatingByte(0, count: requiredSpace)
+        // Move the data forward by that many bytes, to make space at the front
+        self.withVeryUnsafeMutableBytes { pointer in


could we avoid *Unsafe* here?

Not really: this is the safe wrapper around the access.

Ultimately this is implementing memmove on ByteBuffer. Implementing it safely is somewhere between very hard and impossible using the APIs we currently have. We can run through the options:

Bytewise copy using writeInteger. This is going to be very costly for large moves, which is the concern we have in this method.

Manually-autovectorised copy using writeMultipleInteger and wider integers. This can work, but it's still going to be very slow compared to memmove and it will be much more complex code as we have to handle a wide range of edge-cases.

Move using writeImmutableBuffer. This will, unfortunately, trigger a CoW.

Move using ByteBufferView. This will also trigger a CoW.

I fundamentally can't see an implementation here that performs well enough to be used on larger buffers, which is also the circumstance in which it is most likely that we'll need a move. But I'm open to other options.

Okay, if this is necessary functionality, can we add it to ByteBuffer-core.swift and use in other places too? If my memory serves me right, then @glbrntt added some memory-moving API already a few years back, might be wrong about that though

Close enough, I added copyBytes(at:to:length:) 5 years ago as part of making ByteBufferView mutable. Good memory!

https://github.com/apple/swift-nio/blame/282f5935cf3352b3d026c35eb57cb3619dd9536f/Sources/NIOCore/ByteBuffer-core.swift#L1077

@hamzahrmalik isn't that exactly what we need? memmove == copying bytes too

Yeah, this looks like we could use it here.

Yep, this works perfectly

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

Lukasa · 2024-09-23T16:38:45Z

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

+    /// If you use write more or less than this many bytes, we wll need to move bytes around to make that possible.
+    /// Therefore, you may decide to encode differently to use more bytes than you actually would need, if it is possible for you to do so.
+    /// That way, you waste the reserved bytes, but improve writing performance.
+    func writeIntegerWithReservedCapacity(


George's note here is good, we can probably remove the note in the name.

Lukasa · 2024-09-23T16:39:05Z

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

+        to buffer: inout ByteBuffer
+    ) -> Int
+
+    /// An estimate of the bytes required to write integers using ths strategy.


Suggested change

/// An estimate of the bytes required to write integers using ths strategy.

/// An estimate of the bytes required to write integers using this strategy.

Lukasa · 2024-09-23T16:39:27Z

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

+    var requiredBytesHint: Int { get }
+
+    /// Write an integer to a buffer. Move the writer index to after the written integer.
+    /// Call this function if you have already reserved some capacity for an integer to be written.


The tone of this sentence seems wrong: it's an instruction to the caller, but the caller is us.

Lukasa · 2024-09-23T16:39:53Z

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

+
+    /// Write an integer to a buffer. Move the writer index to after the written integer.
+    /// Call this function if you have already reserved some capacity for an integer to be written.
+    /// Implementors should consider using a less efficient encoding, if possible,to fit exactly within the reserved capacity.


Suggested change

/// Implementors should consider using a less efficient encoding, if possible,to fit exactly within the reserved capacity.

/// Implementers should consider using a less efficient encoding, if possible,to fit exactly within the reserved capacity.

Lukasa · 2024-09-23T16:39:59Z

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

+    /// Call this function if you have already reserved some capacity for an integer to be written.
+    /// Implementors should consider using a less efficient encoding, if possible,to fit exactly within the reserved capacity.
+    /// Otherwise, callers may need to shift bytes to reconcile the difference.
+    /// It is up to the implementor to find the balance between performance and size.


Suggested change

/// It is up to the implementor to find the balance between performance and size.

/// It is up to the implementer to find the balance between performance and size.

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

Lukasa · 2024-09-23T16:54:45Z

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

+    /// Read a binary encoded integer, moving the `readerIndex` appropriately.
+    /// If there are not enough bytes, nil is returned.
+    @inlinable
+    public mutating func readEncodedInteger<Strategy: NIOBinaryIntegerEncodingStrategy>(_ strategy: Strategy) -> Int? {


These should probably be generic over FixedWidthInteger, the way the existing readInteger and writeInteger functions are.

Lukasa · 2024-09-30T12:53:18Z

Sources/NIOCore/ByteBuffer-quicBinaryEncodingStrategy.swift

+//===----------------------------------------------------------------------===//
+
+extension ByteBuffer {
+    public struct QUICBinaryEncodingStrategy: NIOBinaryIntegerEncodingStrategy {


It'd be nice to have a doc comment here.

Lukasa · 2024-09-30T12:53:28Z

Sources/NIOCore/ByteBuffer-quicBinaryEncodingStrategy.swift

+
+        /// - Parameter requiredBytesHint: An estimate of the bytes required to write integers using this strategy
+        @inlinable
+        public init(requiredBytesHint: Int) {


Do we think it's useful to make this configurable?

In some cases, a user might have a rough idea of how many bytes they're expecting to write. In such cases, allowing them to use a smaller number would result in saved bytes

An alternative would be, make this not configurable, but then add a parameter to ByteBuffer/writeLengthPrefixed(strategy:writeData:)

leaning towards the alternative
Having the requiredBytesHint parameter here is quite confusing

glbrntt

This mostly looks really good. Most of my comments are doc nits but a couple are minor API tweaks that I think we should fix.

glbrntt · 2024-09-24T10:11:59Z

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

+/// Describes a way to encode and decode an integer as bytes
+/// For more information, see <doc:ByteBuffer-lengthPrefix>


Is this once sentence or two? We need some punctuation between the two otherwise it'll be rendered as "... bytes For more information, ..."

glbrntt · 2024-09-25T14:13:04Z

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

+
+    /// Write an integer to a buffer. Move the writer index to after the written integer.
+    /// - Parameters:
+    ///    - integer: The type of the integer to write.


Suggested change

/// - integer: The type of the integer to write.

/// - integer: The integer to write.

glbrntt · 2024-09-30T14:45:46Z

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

+
+    /// Write an integer to a buffer. Move the writer index to after the written integer.
+    /// This function will be called when an integer needs to be written, and some capacity has already been reserved for it.
+    /// Implementers should consider using a less efficient encoding, if possible,to fit exactly within the reserved capacity.


Suggested change

/// Implementers should consider using a less efficient encoding, if possible,to fit exactly within the reserved capacity.

/// Implementers should consider using a less efficient encoding, if possible, to fit exactly within the reserved capacity.

glbrntt · 2024-09-30T14:50:33Z

Sources/NIOCore/Docs.docc/ByteBuffer-lengthPrefix.md

+integer encodings, in which smaller numbers can be encoded in fewer bytes. The first 2 bits of the first byte indicate
+how many further bytes should be read.


I think the QUIC example is great but don't think we need the detail about how the QUIC strategy works.

Suggested change

integer encodings, in which smaller numbers can be encoded in fewer bytes. The first 2 bits of the first byte indicate

how many further bytes should be read.

integer encodings, in which smaller numbers can be encoded in fewer bytes.

glbrntt · 2024-09-30T14:52:24Z

Sources/NIOCore/Docs.docc/ByteBuffer-lengthPrefix.md

+
+- Encode the length of the integer into the integer itself when writing, so it knows how many bytes to read when
+  reading. This is what QUIC does.
+- Always use the same length, e.g. a simple strategy which always writes the integer as a UInt64.


Suggested change

- Always use the same length, e.g. a simple strategy which always writes the integer as a UInt64.

- Always use the same length, e.g. a simple strategy which always writes the integer as a `UInt64`.

glbrntt · 2024-09-30T16:10:25Z

Sources/NIOCore/Docs.docc/ByteBuffer-lengthPrefix.md

+    - If the length ends up needing more bytes than we had reserved, shuffle the data forward to make space
+
+This code will be most performant when the `requiredBytesHint` is exactly correct, because it will avoid needing to
+shuffle any bytes. With that in mind, we can actually make one more optimisation: when we call the writeInteger function


Suggested change

shuffle any bytes. With that in mind, we can actually make one more optimisation: when we call the writeInteger function

shuffle any bytes. With that in mind, we can actually make one more optimisation: when we call the `writeInteger` function

glbrntt · 2024-09-30T16:12:35Z

Sources/NIOCore/Docs.docc/ByteBuffer-lengthPrefix.md

+) -> Int
+```
+
+Many strategies will not be able to do anything useful with the additional `reservedCapacity` parameter. For e.g., in


nit: "e.g." is the abbreviation of "exempli gratia" which means "for the sake of example", so no need to write "For e.g.", Just use "e.g." or "for example"

Suggested change

Many strategies will not be able to do anything useful with the additional `reservedCapacity` parameter. For e.g., in

Many strategies will not be able to do anything useful with the additional `reservedCapacity` parameter. For example, in

glbrntt · 2024-09-30T16:16:09Z

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

+    public mutating func readEncodedInteger<Strategy: NIOBinaryIntegerEncodingStrategy, Integer: FixedWidthInteger>(
+        _ strategy: Strategy,
+        as: Integer.Type = Integer.self
+    ) -> Integer? {


readEncodedInteger(.quic) is a bit odd when using type inference (and is different to writeEncodedInteger(42, strategy: .quic)).

I think this should be spelled as readEncodedInteger(as:strategy:) which works well with or without type inference and mirrors the write API nicely.

glbrntt · 2024-09-30T16:16:55Z

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

+    }
+
+    /// Prefixes bytes written by `writeData` with the number of bytes written.
+    /// The number of bytes written is encoded using `strategy`


Suggested change

/// The number of bytes written is encoded using `strategy`

/// The number of bytes written is encoded using `strategy`.

glbrntt · 2024-09-30T16:18:44Z

Sources/NIOCore/ByteBuffer-binaryEncodedLengthPrefix.swift

+    /// If there are not enough bytes to read the full slice, the readerIndex will stay unchanged.
+    @inlinable
+    public mutating func readLengthPrefixedSlice<Strategy: NIOBinaryIntegerEncodingStrategy>(
+        _ strategy: Strategy


Not sure about dropping the label on the strategy here, I think we should keep the strategy label.

glbrntt

Looks good modulo one doc nit!

glbrntt · 2024-09-30T17:56:27Z

Sources/NIOCore/ByteBuffer-quicBinaryEncodingStrategy.swift

@@ -129,10 +131,14 @@ extension ByteBuffer {

 extension NIOBinaryIntegerEncodingStrategy where Self == ByteBuffer.QUICBinaryEncodingStrategy {
    @inlinable
+    /// Encodes bytes as defined in RFC 9000 § 16
+    /// - Parameter requiredBytesHint: An estimate of the bytes required to write integers using this strategy
+    /// - Returns: <#description#>


This one needs filling out, <#description#>

ehaydenr · 2024-09-30T21:42:33Z

Sources/NIOCore/ByteBuffer-quicBinaryEncodingStrategy.swift

+        }
+
+        @usableFromInline
+        func bytesNeededForInteger<IntegerType: FixedWidthInteger>(_ integer: IntegerType) -> Int {


I think exposing this logic publicly would be useful. Consider a case where I want to write multiple variable length integers to a buffer - how can I appropriately size the buffer's capacity ahead of time? I can assume they'll all need 8 bytes, but then I'm going to over allocate.

var buffer = allocator.buffer(capacity: enoughRoomForTwoVarints) buffer.writeInteger(myInt, strategy: .quic) buffer.writeInteger(myOtherInt, strategy: .quic)

Lukasa

Nice one, let's get this landed.

finagolfin · 2024-10-03T10:09:00Z

Tests/NIOCoreTests/ByteBufferQUICBinaryEncodingStrategyTests.swift

+        var buffer = ByteBuffer()
+        let strategy = ByteBuffer.QUICBinaryEncodingStrategy.quic
+        let bytesWritten = strategy.writeInteger(
+            0b00000010_00011001_01111100_01011110_11111111_00010100_11101000_10001100,


@hamzahrmalik, this line and a couple more writes below broke compilation of these tests for 32-bit platforms like Android armv7 on my Android CI.

@finagolfin @Lukasa @FranzBusch can we integrate the Android & 32 bit CIs into NIO's own github actions?

Franz and I discussed getting Android tested on this CI, I'm aiming to have something for that in the coming months. However, that just builds the source for armv7 but wouldn't run the tests.

If you have a 32-bit CI like watchOS or embedded that you could add quickly, that may be a better first step to get 32-bit platforms tested.

I've raised a PR to fix this: #2904

… systems (#2904) This is a followup to #2867 Some of the literals used in the test cases were too big to fit in an Int32 This is fine on 64-bit systems, because the literals are considered as `Int`, which is Int64 on those systems However, on 32-bit systems, those literals are considered as Int64 Change: Add `as Int64` where needed, to tell the compiler we want these literals to be treated as Int64, which should allow these tests to run on 32 bit systems too

hamzahrmalik added the 🆕 semver/minor Adds new public API. label Sep 4, 2024

Add functions for reading and writing length-prefixed data with custo…

74ce004

…misable encodings for the length

hamzahrmalik force-pushed the binary_integer_length_encoding branch from b181d12 to 74ce004 Compare September 4, 2024 18:39

glbrntt requested changes Sep 5, 2024

View reviewed changes

code review

03a6a48

glbrntt requested changes Sep 10, 2024

View reviewed changes

hamzahrmalik added 2 commits September 11, 2024 12:46

Rename things for clarity

1cca933

Fix

e0d8b95

hamzahrmalik mentioned this pull request Sep 11, 2024

Add functions for reading and writing quic variable-length encoded integers #2859

Closed

Reserve bytes before writing

1491330

weissi reviewed Sep 12, 2024

View reviewed changes

MaxDesiatov reviewed Sep 12, 2024

View reviewed changes

weissi reviewed Sep 12, 2024

View reviewed changes

hamzahrmalik added 3 commits September 12, 2024 20:02

More tests

8154352

Cleanup

082c70a

Cleanup and docs

a006166

Lukasa reviewed Sep 23, 2024

View reviewed changes

hamzahrmalik added 4 commits September 23, 2024 19:25

Rename writeIntegerWithReservedCapacity

11163e7

Doc changes

a925977

Make readEncodedInteger generic over integer type

2008d02

Fix docs

cda29cf

hamzahrmalik requested review from glbrntt and Lukasa September 24, 2024 09:06

Lukasa reviewed Sep 30, 2024

View reviewed changes

glbrntt requested changes Sep 30, 2024

View reviewed changes

Code review

15bb9e1

glbrntt approved these changes Sep 30, 2024

View reviewed changes

ehaydenr reviewed Sep 30, 2024

View reviewed changes

Doc fix

4b134a4

hamzahrmalik added 3 commits October 1, 2024 08:23

Make bytesNeededForInteger public

436e74a

Remove unused type

36d2d01

code review

43e4fa7

Lukasa approved these changes Oct 2, 2024

View reviewed changes

Merge branch 'main' into binary_integer_length_encoding

0a947dd

hamzahrmalik enabled auto-merge (squash) October 2, 2024 09:27

hamzahrmalik merged commit fb87d5e into apple:main Oct 2, 2024
28 of 29 checks passed

finagolfin reviewed Oct 3, 2024

View reviewed changes

hamzahrmalik mentioned this pull request Oct 4, 2024

Make ByteBufferQUICBinaryEncodingStrategyTests compatible with 32-bit systems #2904

Merged

	/// An estimate of the bytes required to write integers using ths strategy.
	/// An estimate of the bytes required to write integers using this strategy.

	/// Implementors should consider using a less efficient encoding, if possible,to fit exactly within the reserved capacity.
	/// Implementers should consider using a less efficient encoding, if possible,to fit exactly within the reserved capacity.

	/// It is up to the implementor to find the balance between performance and size.
	/// It is up to the implementer to find the balance between performance and size.

		/// Describes a way to encode and decode an integer as bytes
		/// For more information, see <doc:ByteBuffer-lengthPrefix>

	/// - integer: The type of the integer to write.
	/// - integer: The integer to write.

		integer encodings, in which smaller numbers can be encoded in fewer bytes. The first 2 bits of the first byte indicate
		how many further bytes should be read.

	- Always use the same length, e.g. a simple strategy which always writes the integer as a UInt64.
	- Always use the same length, e.g. a simple strategy which always writes the integer as a `UInt64`.

	shuffle any bytes. With that in mind, we can actually make one more optimisation: when we call the writeInteger function
	shuffle any bytes. With that in mind, we can actually make one more optimisation: when we call the `writeInteger` function

	Many strategies will not be able to do anything useful with the additional `reservedCapacity` parameter. For e.g., in
	Many strategies will not be able to do anything useful with the additional `reservedCapacity` parameter. For example, in

	/// The number of bytes written is encoded using `strategy`
	/// The number of bytes written is encoded using `strategy`.

Add functions for reading and writing length-prefixed data with customizable encodings for the length #2867

Add functions for reading and writing length-prefixed data with customizable encodings for the length #2867

Conversation

hamzahrmalik commented Sep 4, 2024

Motivation:

Modifications:

glbrntt 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

glbrntt 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

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

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

glbrntt 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

Choose a reason for hiding this comment

glbrntt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa 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