internal/marshal: marshal is extremely inefficient #38

New Issue

Open

opened 2020-03-30 13:42:05 +00:00 by SamWhited · 0 comments

SamWhited commented

2020-03-30 13:42:05 +00:00

Owner

The code from internal/marshal is incredibly inefficient due to limitations in encoding/xml because it currently has to create a buffer, encode XML, then decode the XML it just created from the buffer:

ad06d1aac2/internal/marshal/encode.go (L18-L26)

To fix this we should either:

Look into fixing encoding/xml to also support token encoders (see previous experiments at https://golang.org/cl/127476 and https://golang.org/cl/127415)
or re-implement encoding in terms of the mellium.im/xmlstream module and run it against the Go standard library tests to make sure it outputs the same thing

If we do option 2, it should be developed in internal/ and then moved to xmlstream once it has been verified and used more extensively. If we do option 1 a temporary workaround that uses the token encoder on versions of Go that support it and falls back to the current hack should be added and an issue created reminding us to remove the hack when all supported versions of Go contain the token writer APIs.

Other suggestions for how this could be improved are also welcome.

Remove the following line from the docs when fixed:

8ee3bc1f84/internal/marshal/encode.go (L16)

The code from [`internal/marshal`] is incredibly inefficient due to limitations in [`encoding/xml`] because it currently has to create a buffer, encode XML, then decode the XML it just created from the buffer: https://github.com/mellium/xmpp/blob/ad06d1aac2abbec1db5a92fbed3d9a4c8665c7f4/internal/marshal/encode.go#L18-L26 To fix this we should either: 1. Look into fixing `encoding/xml` to also support token encoders (see previous experiments at https://golang.org/cl/127476 and https://golang.org/cl/127415) 2. or re-implement encoding in terms of the [`mellium.im/xmlstream`] module and run it against the Go standard library tests to make sure it outputs the same thing If we do option 2, it should be developed in `internal/` and then moved to `xmlstream` once it has been verified and used more extensively. If we do option 1 a temporary workaround that uses the token encoder on versions of Go that support it and falls back to the current hack should be added and an issue created reminding us to remove the hack when all supported versions of Go contain the token writer APIs. Other suggestions for how this could be improved are also welcome. Remove the following line from the docs when fixed: https://github.com/mellium/xmpp/blob/8ee3bc1f84d3f5a7e907e651253f51eb910b9018/internal/marshal/encode.go#L16 [`internal/marshal`]: https://pkg.go.dev/mellium.im/xmpp@v0.16.0/internal/marshal?tab=doc [`encoding/xml`]: https://golang.org/pkg/encoding/xml/ [`mellium.im/xmlstream`]: https://pkg.go.dev/mellium.im/xmlstream?tab=doc