Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

encoding: add AppendText and AppendBinary #62384

Closed
dsnet opened this issue Aug 30, 2023 · 50 comments
Closed

encoding: add AppendText and AppendBinary #62384

dsnet opened this issue Aug 30, 2023 · 50 comments

Comments

@dsnet
Copy link
Member

dsnet commented Aug 30, 2023

The MarshalText and MarshalBinary methods are a trash factory.
They create a short-lived string that is almost always garbage collectable shortly after creation.
Furthermore, the caller almost always copied the result into another buffer, resulting in further waste.

I propose the addition of the following interfaces to the "encoding" package:

// TextAppender is the interface implemented by an object
// that can append the textual representation of itself.
// If a type implements both [TextAppender] and [TextMarshaler],
// then v.MarshalText() must be semantically identical to v.AppendText(nil).
type TextAppender interface {
    // AppendText appends the textual representation of itself to the end of b
    // (allocating a larger slice if necessary) and returns the updated slice.
    //
    // Implementations must not retain b, nor mutate any bytes within b[:len(b)].
    AppendText(b []byte) ([]byte, error)
}

type BinaryAppender interface {
    AppendBinary([]byte) ([]byte, error)
}

In addition to the above, we would update all types in the stdlib that implement MarshalText or MarshalBinary to have the Append equivalent (and switch the Marshal implementation to just call the Append variant). Also, we would teach all stdlib libraries that looks for TextMarshaler or BinaryMarshaler to also look for the append (and unmarshal) variant.

\cc @bradfitz @maisem

@dsnet dsnet added the Proposal label Aug 30, 2023
@gopherbot gopherbot added this to the Proposal milestone Aug 30, 2023
@dsnet
Copy link
Member Author

dsnet commented Aug 30, 2023

This is motivated by netip.Addr.MarshalText being a non-trivial amount of slowdown in a large JSON object.

@dsnet
Copy link
Member Author

dsnet commented Aug 30, 2023

Note that I did not propose json.Appender. There are other possible API designs that allow for streaming JSON encoding and decoding which would obviate the need for it. A separate proposal or discussion will be made for this in the future.

@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Aug 30, 2023
@dsnet
Copy link
Member Author

dsnet commented Aug 30, 2023

Analyzing all code on the module proxy from 2023-07-01:

I do not believe there are any backwards compatibility issues with this change.

[email protected]/pango/pango.go:95: func (n *Node) AppendText(texts ...string) *Node {
bitbucket.org/afiszone/[email protected]/gtk/combo_box.go:263: func (v *ComboBoxText) AppendText(text string) {
code.afis.me/[email protected]/gtk/combo_box.go:354: func (v *ComboBoxText) AppendText(text string) {
code.aliyun.com/newrank-tools/[email protected]/api/markdown/markdown.go:47: func (md *Markdown) AppendText(content string, params ...bool) *Markdown {
codeberg.org/japh/[email protected]/table.go:149: func (tbl *Table) AppendText(textLines ...string) {
git.gammaspectra.live/P2Pool/[email protected]/monero/transaction/extra.go:54: func (t *ExtraTags) AppendBinary(preAllocatedBuf []byte) (buf []byte, err error) {
git.gammaspectra.live/P2Pool/[email protected]/monero/transaction/extra.go:124: func (t *ExtraTag) AppendBinary(preAllocatedBuf []byte) ([]byte, error) {
git.gammaspectra.live/P2Pool/[email protected]/monero/transaction/output.go:78: func (s *Outputs) AppendBinary(preAllocatedBuf []byte) (data []byte, err error) {
git.gammaspectra.live/P2Pool/[email protected]/p2pool/sidechain/sidedata.go:52: func (b *SideData) AppendBinary(preAllocatedBuf []byte, version ShareVersion) (buf []byte, err error) {
git.milar.in/milarin/[email protected]/rune.go:46: func (t *Text) AppendText(txt *Text) {
git.tordarus.net/Tordarus/[email protected]/rune.go:46: func (t *Text) AppendText(txt *Text) {
git.wit.org/wit/[email protected]/common.go:91: func (n *Node) AppendText(str string) {
gitee.com/scpro/[email protected]/gtk/combo_box.go:364: func (v *ComboBoxText) AppendText(text string) {
gitee.com/zhouhailin/[email protected]/hutool/io/io_util.go:27: func (f *IoUtil) AppendText(pathname string, text string) error {
github.com/45638654564654/[email protected]/v1/ao/internal/bson/bson.go:43: func (b *Buffer) AppendBinary(k string, v []byte) {
github.com/474420502/[email protected]/dom/node_element.go:62: func (n *Element) AppendText(s string) error {
github.com/BlackMocca/[email protected]/x/bsonx/bsoncore/bson_arraybuilder.go:94: func (a *ArrayBuilder) AppendBinary(subtype byte, b []byte) *ArrayBuilder {
github.com/BlackMocca/[email protected]/x/bsonx/bsoncore/bson_documentbuilder.go:82: func (db *DocumentBuilder) AppendBinary(key string, subtype byte, b []byte) *DocumentBuilder {
github.com/Existed/[email protected]/dom/node_element.go:62: func (n *Element) AppendText(s string) error {
github.com/FabianWe/[email protected]/etherpadlite.go:308: func (pad *EtherpadLite) AppendText(ctx context.Context, padID, text interface{}) (*Response, error) {
github.com/Gipcomp/[email protected]/textedit.go:245: func (te *TextEdit) AppendText(value string) {
github.com/GoTyro/[email protected]/textedit.go:241: func (te *TextEdit) AppendText(value string) {
github.com/GoTyro/[email protected]/examples/logview/logview.go:55: func (lv *LogView) AppendText(value string) {
github.com/KemalovMaulen/[email protected]/dom/node_element.go:62: func (n *Element) AppendText(s string) error {
github.com/Skarm/[email protected]/textedit.go:94: func (te *TextEdit) AppendText(value string) {
github.com/Skarm/[email protected]/examples/logview/logview.go:63: func (lv *LogView) AppendText(value string) {
github.com/StephanVerbeeck/[email protected]/textedit.go:217: func (te *TextEdit) AppendText(value string) {
github.com/StephanVerbeeck/[email protected]/examples/logview/logview.go:63: func (lv *LogView) AppendText(value string) {
github.com/adejoux/[email protected]+incompatible/nmon/nmon.go:42: func (nmon *Nmon) AppendText(text string) {
github.com/admpub/[email protected]/gui/logview.go:59: func (lv *LogView) AppendText(value string) {
github.com/agl/[email protected]/gtk/gtk.go:5447: func (v *GtkComboBoxText) AppendText(text string) {
github.com/anoshenko/[email protected]/editView.go:357: func (edit *editViewData) AppendText(text string) {
github.com/appoptics/[email protected]/v1/ao/internal/bson/bson.go:43: func (b *Buffer) AppendBinary(k string, v []byte) {
github.com/auroralaboratories/[email protected]/gtk/combo_box.go:229: func (v *ComboBoxText) AppendText(text string) {
github.com/bakharal/[email protected]/x/bsonx/bsoncore/bson_arraybuilder.go:94: func (a *ArrayBuilder) AppendBinary(subtype byte, b []byte) *ArrayBuilder {
github.com/bakharal/[email protected]/x/bsonx/bsoncore/bson_documentbuilder.go:82: func (db *DocumentBuilder) AppendBinary(key string, subtype byte, b []byte) *DocumentBuilder {
github.com/bennicholls/[email protected]/burl/textbox.go:29: func (t *Textbox) AppendText(txt string) {
github.com/blorticus/[email protected]/tpcli.go:391: func (panel *outputPanel) AppendText(s string) {
github.com/bontibon/[email protected]/manipulation.go:141: func (ui *UI) AppendText(widget, text string) {
github.com/bytbox/[email protected]/gtk-3.0/gtk.go:14947: func (this0 *ComboBoxText) AppendText(text0 string) {
github.com/chai2010/[email protected]/textedit.go:240: func (te *TextEdit) AppendText(value string) {
github.com/chai2010/[email protected]/examples/logview/logview.go:55: func (lv *LogView) AppendText(value string) {
github.com/clockworkpi/[email protected]/sysgo/UI/textarea.go:80: func (self *Textarea) AppendText(alphabet string) {
github.com/clockworksoul/[email protected]/edit.go:259: func (w *EditClient) AppendText(s string) *EditClient {
github.com/cloudfly/[email protected]/pkg/events/channel.go:61: func (spec ChannelSpec) AppendText(data map[string]interface{}) string {
github.com/cloudfly/[email protected]/pkg/events/channel.go:61: func (spec ChannelSpec) AppendText(data map[string]interface{}) string {
github.com/cloudradar-monitoring/[email protected]/pkg/winui/winui_logview.go:64: func (lv *LogView) AppendText(value string) {
github.com/coyim/[email protected]/gtk_mock/combo_box_text.go:7: func (*MockComboBoxText) AppendText(v1 string) {
github.com/coyim/[email protected]/gtka/combo_box_text.go:31: func (v *comboBoxText) AppendText(v1 string) {
github.com/coyim/[email protected]/gtk/combo_box_text.go:7: func (m *MockComboBoxText) AppendText(v1 string) {
github.com/d2r2/[email protected]/gtk/gtk.go:5223: func (v *ComboBox) AppendText(text string) {
github.com/d2r2/[email protected]/gtk/gtk.go:5309: func (v *ComboBoxText) AppendText(text string) {
github.com/d2r2/[email protected]/gtk/combobox.go:240: func (v *ComboBoxText) AppendText(text string) {
github.com/davidli2010/[email protected]/bson/bsonarraybuilder.go:103: func (a *BsonArrayBuilder) AppendBinary(value Binary) *BsonArrayBuilder {
github.com/davidli2010/[email protected]/bson/bsonbuilder.go:183: func (b *BsonBuilder) AppendBinary(name string, value Binary) *BsonBuilder {
github.com/diamondburned/gotk4/[email protected]/gtk/v3/gtkcomboboxtext.go:235: func (comboBox *ComboBoxText) AppendText(text string) {
github.com/diamondburned/gotk4/[email protected]/gtk/v4/gtkcomboboxtext.go:213: func (comboBox *ComboBoxText) AppendText(text string) {
github.com/dimansk12/[email protected]/dom/node_element.go:62: func (n *Element) AppendText(s string) error {
github.com/dontpanic92/[email protected]/wx/wx_darwin.go:84776: func (arg1 SwigClassTextEntry) AppendText(arg2 string) {
github.com/dontpanic92/[email protected]/wx/wx_darwin.go:245455: func (arg1 SwigClassStyledTextCtrl) AppendText(arg2 string) {
github.com/dontpanic92/[email protected]/wx/wx_darwin.go:290453: func (arg1 SwigClassRichTextCtrl) AppendText(arg2 string) {
github.com/dontpanic92/[email protected]/wx/wx_linux.go:84736: func (arg1 SwigClassTextEntry) AppendText(arg2 string) {
github.com/dontpanic92/[email protected]/wx/wx_linux.go:245543: func (arg1 SwigClassStyledTextCtrl) AppendText(arg2 string) {
github.com/dontpanic92/[email protected]/wx/wx_linux.go:288912: func (arg1 SwigClassRichTextCtrl) AppendText(arg2 string) {
github.com/dontpanic92/[email protected]/wx/wx_windows.go:85111: func (arg1 SwigClassTextEntry) AppendText(arg2 string) {
github.com/dontpanic92/[email protected]/wx/wx_windows.go:246501: func (arg1 SwigClassStyledTextCtrl) AppendText(arg2 string) {
github.com/dontpanic92/[email protected]/wx/wx_windows.go:289885: func (arg1 SwigClassRichTextCtrl) AppendText(arg2 string) {
github.com/ebitenui/[email protected]/widget/textarea.go:336: func (l *TextArea) AppendText(value string) {
github.com/electricface/[email protected]/gtk-3.0/gtk_auto.go:15477: func (v ComboBoxText) AppendText(text string) {
github.com/friendstechday/[email protected]/converter/java/javaDoc/comment.go:11: func (c *Comment) AppendText(text string) {
github.com/friendstechday/[email protected]/converter/java/javaDoc/param.go:13: func (p *Param) AppendText(text string) {     
github.com/gangming/[email protected]/textedit.go:240: func (te *TextEdit) AppendText(value string) {
github.com/gangming/[email protected]/examples/logview/logview.go:55: func (lv *LogView) AppendText(value string) {
github.com/gimalay/[email protected]/binx.go:78: func (e *page) AppendBinary(data []byte) (bool, error) {
github.com/gimalay/[email protected]/binx.go:94: func (c *Count) AppendBinary([]byte) error {
github.com/gimalay/[email protected]/binx_test.go:37: func (e *indexable) AppendBinary(data []byte) (bool, error)  { return false, json.Unmarshal(data, e) }
github.com/gimalay/[email protected]/binx_test.go:52: func (e *indexableSlice) AppendBinary(data []byte) (bool, error) {
github.com/godot-go/[email protected]/pkg/gdextension/classes.gen.go:341178: func (cx *RichTextLabelImpl) AppendText(
github.com/gofaith/[email protected]/textedit.go:240: func (te *TextEdit) AppendText(value string) {
github.com/gofaith/[email protected]/examples/logview/logview.go:55: func (lv *LogView) AppendText(value string) {
github.com/goki/[email protected]/giv/textbuf.go:707: func (tb *TextBuf) AppendText(text []byte, signal bool) *textbuf.Edit {
github.com/gontikr99/[email protected]/controller/gui/logview.go:60: func (lv *LogView) AppendText(value string) {
github.com/gotk3/[email protected]/gtk/combo_box.go:364: func (v *ComboBoxText) AppendText(text string) {
github.com/hegeng1212/[email protected]/gui/logview.go:59: func (lv *LogView) AppendText(value string) {
github.com/henrylee2cn/[email protected]/gui/logview.go:59: func (lv *LogView) AppendText(value string) {
github.com/ianatha/[email protected]/textedit.go:240: func (te *TextEdit) AppendText(value string) {
github.com/ianatha/[email protected]/examples/logview/logview.go:55: func (lv *LogView) AppendText(value string) {
github.com/icowan/[email protected]/textedit.go:240: func (te *TextEdit) AppendText(value string) {
github.com/icowan/[email protected]/examples/logview/logview.go:55: func (lv *LogView) AppendText(value string) {
github.com/ingmardrewing/[email protected]/fs/html.go:116: func (n *htmlNode) AppendText(txt string) node {
github.com/ionous/[email protected]/metal/lists.go:36: func (ar arrayValues) AppendText(v string) error {
github.com/ionous/[email protected]/metal/panicValue.go:31: func (p panicValue) AppendText(string) error {
github.com/ionous/[email protected]/runtime/internal/gameList.go:126: func (l *gameList) AppendText(v string) {
github.com/ionous/[email protected]/runtime/internal/nullList.go:13: func (n nullList) AppendText(string)         {}
github.com/iquanxin/[email protected]/textedit.go:241: func (te *TextEdit) AppendText(value string) {
github.com/jannson/[email protected]/textedit.go:240: func (te *TextEdit) AppendText(value string) {
github.com/jannson/[email protected]/examples/logview/logview.go:55: func (lv *LogView) AppendText(value string) {
github.com/jason-wj/[email protected]/gui/logview.go:59: func (lv *LogView) AppendText(value string) {
github.com/joeirimpan/[email protected]/dom/node_element.go:62: func (n *Element) AppendText(s string) error {
github.com/johnbartholomew/[email protected]/gtk/gtk.go:4955: func (v *ComboBox) AppendText(text string) {
github.com/johnbartholomew/[email protected]/gtk/gtk.go:5041: func (v *ComboBoxText) AppendText(text string) {
github.com/jopbrown/[email protected]/lib/gtk/combo.go:42: func (obj *ComboBoxText) AppendText(text string) {
github.com/jopbrown/[email protected]/lib/gtk/combo.gtk2.go:10: func (obj *ComboBox) AppendText(text string) {
github.com/justinyaoqi/[email protected]/gui/logview.go:65: func (lv *LogView) AppendText(value string) {
github.com/jzs/[email protected]/textedit.go:147: func (te *TextEdit) AppendText(value string) {
github.com/jzs/[email protected]/examples/logview/logview.go:63: func (lv *LogView) AppendText(value string) {
github.com/karashiiro/[email protected]/pkg/colortext/builder.go:11: func (b *ColorTextBuilder) AppendText(text string) *ColorTextBuilder {
github.com/kckrinke/[email protected]/gtk/gtk.go:6126: func (v *ComboBox) AppendText(text string) {
github.com/kckrinke/[email protected]/gtk/gtk.go:6221: func (v *ComboBoxText) AppendText(text string) {
github.com/ktye/[email protected]/editor/edit.go:47: func (e *Edit) AppendText(s string) {
github.com/kumakichi/[email protected]/textedit.go:94: func (te *TextEdit) AppendText(value string) {
github.com/kumakichi/[email protected]/examples/logview/logview.go:63: func (lv *LogView) AppendText(value string) {
github.com/kvark128/[email protected]/textedit.go:240: func (te *TextEdit) AppendText(value string) {
github.com/kvark128/[email protected]/examples/logview/logview.go:55: func (lv *LogView) AppendText(value string) {
github.com/lazyshot/[email protected]/gtk/gtk.go:2629: func (v *ComboBoxText) AppendText(text string) {
github.com/leaanthony/[email protected]/gtk/gtk.go:6189: func (v *ComboBox) AppendText(text string) {
github.com/leaanthony/[email protected]/gtk/gtk.go:6284: func (v *ComboBoxText) AppendText(text string) {
github.com/lestrrat/[email protected]/dom/node_element.go:62: func (n *Element) AppendText(s string) error {
github.com/lestrrat-go/[email protected]/dom/node_element.go:64: func (n *Element) AppendText(s string) error {
github.com/liwh011/[email protected]/message.go:44: func (m *Message) AppendText(t string) {
github.com/loyalpartner/[email protected]/stream.go:31: func (n *nigoriStream) AppendText(str string) {
github.com/lufuhu/[email protected]/gtk/combo_box.go:364: func (v *ComboBoxText) AppendText(text string) {
github.com/lxn/[email protected]/textedit.go:240: func (te *TextEdit) AppendText(value string) {
github.com/lxn/[email protected]/examples/logview/logview.go:55: func (lv *LogView) AppendText(value string) {
github.com/mad-day/[email protected]/bsoncore/bson_arraybuilder.go:94: func (a *ArrayBuilder) AppendBinary(subtype byte, b []byte) *ArrayBuilder {
github.com/mad-day/[email protected]/bsoncore/bson_documentbuilder.go:82: func (db *DocumentBuilder) AppendBinary(key string, subtype byte, b []byte) *DocumentBuilder {
github.com/marcusolsson/[email protected]/Godeps/_workspace/src/github.com/mattn/go-gtk/gtk/gtk.go:5310: func (v *ComboBox) AppendText(text string) {
github.com/marcusolsson/[email protected]/Godeps/_workspace/src/github.com/mattn/go-gtk/gtk/gtk.go:5396: func (v *ComboBoxText) AppendText(text string) {
github.com/mardukbp/[email protected]/tk/text.go:410: func (w *Text) AppendText(text string) error {
github.com/mattn/[email protected]/gtk/gtk.go:6188: func (v *ComboBox) AppendText(text string) {
github.com/mattn/[email protected]/gtk/gtk.go:6283: func (v *ComboBoxText) AppendText(text string) {
github.com/maurofran/[email protected]/matcher/description.go:70: func (d Description) AppendText(text string) {
github.com/mjm/[email protected]/block/builder.go:96: func (b *Builder) AppendText(text string) {
github.com/mrbear258/[email protected]/gtk/combo_box.go:364: func (v *ComboBoxText) AppendText(text string) {
github.com/muumlover/[email protected]/textedit.go:151: func (te *TextEdit) AppendText(value string) {
github.com/muumlover/[email protected]/examples/logview/logview.go:63: func (lv *LogView) AppendText(value string) {
github.com/natemaia/[email protected]/gtk/combo_box.go:364: func (v *ComboBoxText) AppendText(text string) {
github.com/norisatir/[email protected]/gtk3/gtk3.go:11356: func (self *ComboBoxText) AppendText(text string) {
github.com/novln/[email protected]/encoder_test.go:140: func (encoder *TestEncoder) AppendBinary(value []byte) {}
github.com/novln/[email protected]/encoder/json/types.go:434: func (encoder *Encoder) AppendBinary(value []byte) {
github.com/oldkingnearby/[email protected]/textedit.go:240: func (te *TextEdit) AppendText(value string) {
github.com/oldkingnearby/[email protected]/examples/logview/logview.go:55: func (lv *LogView) AppendText(value string) {
github.com/pauldub/[email protected]/gtk3/gtk3.go:11358: func (self *ComboBoxText) AppendText(text string) {
github.com/pekim/[email protected]/lib/gtk/v-2.24.go:16816: func (recv *ComboBoxText) AppendText(text string) {
github.com/pekim/[email protected]/lib/gtk/v-3.0.go:18683: func (recv *ComboBoxText) AppendText(text string) {
github.com/pekim/[email protected]/lib/gtk/v-3.10.go:19473: func (recv *ComboBoxText) AppendText(text string) {
github.com/pekim/[email protected]/lib/gtk/v-3.12.go:19619: func (recv *ComboBoxText) AppendText(text string) {
github.com/pekim/[email protected]/lib/gtk/v-3.14.go:19877: func (recv *ComboBoxText) AppendText(text string) {
github.com/pekim/[email protected]/lib/gtk/v-3.16.go:19976: func (recv *ComboBoxText) AppendText(text string) {
github.com/pekim/[email protected]/lib/gtk/v-3.18.go:20013: func (recv *ComboBoxText) AppendText(text string) {
github.com/pekim/[email protected]/lib/gtk/v-3.2.go:18901: func (recv *ComboBoxText) AppendText(text string) {
github.com/pekim/[email protected]/lib/gtk/v-3.20.go:20182: func (recv *ComboBoxText) AppendText(text string) {
github.com/pekim/[email protected]/lib/gtk/v-3.22.26.go:20199: func (recv *ComboBoxText) AppendText(text string) {
github.com/pekim/[email protected]/lib/gtk/v-3.22.29.go:20217: func (recv *ComboBoxText) AppendText(text string) {
github.com/pekim/[email protected]/lib/gtk/v-3.22.6.go:20190: func (recv *ComboBoxText) AppendText(text string) {
github.com/pekim/[email protected]/lib/gtk/v-3.22.go:20190: func (recv *ComboBoxText) AppendText(text string) {
github.com/pekim/[email protected]/lib/gtk/v-3.4.go:19167: func (recv *ComboBoxText) AppendText(text string) {
github.com/pekim/[email protected]/lib/gtk/v-3.6.go:19278: func (recv *ComboBoxText) AppendText(text string) {
github.com/pekim/[email protected]/lib/gtk/v-3.8.go:19293: func (recv *ComboBoxText) AppendText(text string) {
github.com/pietroglyph/[email protected]/textedit.go:147: func (te *TextEdit) AppendText(value string) {
github.com/pietroglyph/[email protected]/examples/logview/logview.go:63: func (lv *LogView) AppendText(value string) {
github.com/pirogom/[email protected]/textedit.go:241: func (te *TextEdit) AppendText(value string) {
github.com/planetscale/[email protected]/encoder.go:195: func (enc *prettyEncoder) AppendBinary(value []byte) {
github.com/pritunl/[email protected]/x/bsonx/bsoncore/bson_arraybuilder.go:94: func (a *ArrayBuilder) AppendBinary(subtype byte, b []byte) *ArrayBuilder {
github.com/pritunl/[email protected]/x/bsonx/bsoncore/bson_documentbuilder.go:82: func (db *DocumentBuilder) AppendBinary(key string, subtype byte, b []byte) *DocumentBuilder {
github.com/pwiecz/[email protected]/tk/text.go:410: func (w *Text) AppendText(text string) error {
github.com/pzartem/[email protected]/dom/node_element.go:62: func (n *Element) AppendText(s string) error {
github.com/qsirwyk/[email protected]/textedit.go:240: func (te *TextEdit) AppendText(value string) {
github.com/qsirwyk/[email protected]/examples/logview/logview.go:55: func (lv *LogView) AppendText(value string) {
github.com/raceresult/[email protected]/textedit.go:240: func (te *TextEdit) AppendText(value string) {
github.com/readykit/[email protected]/api.go:25532: func (gdClass RichTextLabel) AppendText(bbcode string)  { methodCall[struct{}](gdClass.obj.get(), methodRichTextLabel[76], &bbcode) }
github.com/reusee/[email protected]/gtk/gtk_traits.go:5592: func (self *TraitComboBoxText) AppendText(text string) {
github.com/richardwilkes/[email protected]/cef/Textfield_gen.go:111: func (d *Textfield) AppendText(text string) {
github.com/romychs/[email protected]/gtk/combobox.go:240: func (v *ComboBoxText) AppendText(text string) {
github.com/rstshardware/[email protected]/textedit.go:241: func (te *TextEdit) AppendText(value string) {
github.com/rstshardware/[email protected]/examples/logview/logview.go:53: func (lv *LogView) AppendText(value string) {
github.com/scjtqs2/[email protected]/textedit.go:240: func (te *TextEdit) AppendText(value string) {
github.com/scjtqs2/[email protected]/examples/logview/logview.go:55: func (lv *LogView) AppendText(value string) {
github.com/tHinqa/[email protected]/gtk/gtk_c.go:1059: func (c *ComboBox) AppendText(text string) { ComboBoxAppendText(c, text) }
github.com/tHinqa/[email protected]/gtk/gtk_c.go:1109: func (c *ComboBoxText) AppendText(text string) { ComboBoxTextAppendText(c, text) }
github.com/tHinqa/[email protected]/gtk/gtk_e.go:194: func (e *Entry) AppendText(text string)            { EntryAppendText(e, text) }
github.com/tailscale/[email protected]/textedit.go:240: func (te *TextEdit) AppendText(value string) {
github.com/tailscale/[email protected]/examples/logview/logview.go:55: func (lv *LogView) AppendText(value string) {
github.com/tiborvass/[email protected]/utils.go:58: func (t text) AppendText(n text) text {
github.com/timelessnesses/[email protected]/tk/text.go:410: func (w *Text) AppendText(text string) error {
github.com/tmaxmax/[email protected]/message.go:98: func (e *Message) AppendText(chunks ...string) {
github.com/turutcrane/[email protected]/capi/cefingo_gen.go:27863: func (self *CTextfieldT) AppendText(
github.com/vikulin/[email protected]/textedit.go:240: func (te *TextEdit) AppendText(value string) {
github.com/vikulin/[email protected]/examples/logview/logview.go:55: func (lv *LogView) AppendText(value string) {
github.com/visualfc/[email protected]/tk/text.go:410: func (w *Text) AppendText(text string) error {
github.com/wangch/[email protected]/textedit.go:94: func (te *TextEdit) AppendText(value string) {
github.com/wangch/[email protected]/examples/logview/logview.go:63: func (lv *LogView) AppendText(value string) {
github.com/wayf-dk/[email protected]/dom/node_element.go:65: func (n *Element) AppendText(s string) error {
github.com/weili71/[email protected]/filex.go:200: func (f *Filex) AppendText(file *os.File, text string) (int, error) {
github.com/whtiehack/[email protected]/edit.go:50: func (e *Edit) AppendText(value string) {
github.com/winxxp/[email protected]/textedit.go:240: func (te *TextEdit) AppendText(value string) {
github.com/winxxp/[email protected]/examples/logview/logview.go:55: func (lv *LogView) AppendText(value string) {
github.com/xlplbo/[email protected]/textedit.go:240: func (te *TextEdit) AppendText(value string) {
github.com/xlplbo/[email protected]/examples/logview/logview.go:38: func (lv *LogView) AppendText(value string) {
github.com/yamnikov-oleg/[email protected]/gtk/gtk.go:5343: func (v *ComboBox) AppendText(text string) {
github.com/yamnikov-oleg/[email protected]/gtk/gtk.go:5430: func (v *ComboBoxText) AppendText(text string) {
github.com/zhangweiii/[email protected]/gui/logview.go:59: func (lv *LogView) AppendText(value string) {
github.com/zhangyiming748/[email protected]/alert.go:67: func (i *Info) AppendText(s string) {
github.com/zurek87/[email protected]/gtk3/gtk3.go:5867: func (v *ComboBoxText) AppendText(text string) {
github.com/zxjsdp/[email protected]/gui/windows/logview.go:63: func (lv *LogView) AppendText(value string) {
gitlab.com/abibino-lab/[email protected]/gui/logview.go:59: func (lv *LogView) AppendText(value string) {
gitlab.com/gitlab-org/security-products/analyzers/common/[email protected]/table.go:30: func (t *Table) AppendText(text string) {
gitlab.com/gitlab-org/security-products/analyzers/common/table/[email protected]/table.go:30: func (t *Table) AppendText(text string) {
go.mongodb.org/[email protected]/x/bsonx/bsoncore/bson_arraybuilder.go:94: func (a *ArrayBuilder) AppendBinary(subtype byte, b []byte) *ArrayBuilder {
go.mongodb.org/[email protected]/x/bsonx/bsoncore/bson_documentbuilder.go:82: func (db *DocumentBuilder) AppendBinary(key string, subtype byte, b []byte) *DocumentBuilder {
modernc.org/[email protected]/text.go:410: func (w *Text) AppendText(text string) error {

@mateusz834
Copy link
Member

This is motivated by netip.Addr.MarshalText being a non-trivial amount of slowdown in a large JSON object.

Just so you now there is netip.Addr.AppendTo, that does the same thing as MarshalText, but appends.

@dsnet
Copy link
Member Author

dsnet commented Aug 31, 2023

There are unfortunately two problems with AppendTo:

  • It doesn't return an error, and so can't implement equivalent semantics as MarshalText
  • It's naming is ambiguous with regard to whether it does "binary" or "textual" marshaling

@mateusz834
Copy link
Member

@dsnet Yes I agree, just wanted to point that there will be now two methods that do the same thing on netip.Addr, probably after this proposal gets accepted the AppendTo should be deprecated.

@mvdan
Copy link
Member

mvdan commented Aug 31, 2023

Scratching my head and wondering how this hasn't been proposed before :) sounds great to me.

@flimzy
Copy link
Contributor

flimzy commented Aug 31, 2023

What are the possible security implications wrt:

// Implementations must not retain b, nor mutate any bytes within b[:len(b)].

I can imagine a malicious, or sloppy, library not following this rule, and introducing nastiness.

Given the other vectors for "nastiness" that already exist throughout the stdlib when using untrusted code, probably not a meaningful consideration, but it does seem a bit icky to me, to expose some other code's data to arbitrary code this way.

@dsnet
Copy link
Member Author

dsnet commented Aug 31, 2023

Given the other vectors for "nastiness" that already exist throughout the stdlib when using untrusted code, probably not a meaningful consideration, but it does seem a bit icky to me, to expose some other code's data to arbitrary code this way.

Agreed. That's the strength of the MarshalText API. It's much harder to hold wrong.

That said, two of the top-three interfaces in Go by far are io.Reader and io.Writer, both of which expose a buffer to the implementation and document:

Implementations must not retain p.

I don't think we're making the situation any worse since encoding.AppendText will be multiple orders of magnitude less popular than io.Reader or io.Writer.

@seebs
Copy link
Contributor

seebs commented Sep 1, 2023

This seems very desireable, the large allocations can be a problem. Although I think in a lot of the cases that are particularly messy, append might not help, because i really need to do an encoder to an io.Writer? But I would still very much like to have this, because iterative loops that are marshaling a bunch of things but only need one at a time are pretty common.

@mateusz834
Copy link
Member

@seebs That is true that something like this would be a nice alternative.

type TextWriter interface {
    WriteText(b io.Writer) error
}

But, this as as side effect would cause another bunch of memory allocations inside each implementation of WriteText, because escape analysis is not great for this use case.

@dsnet
Copy link
Member Author

dsnet commented Sep 1, 2023

Any encoder writing eventually to an io.Writer almost certainly have an intermediate buffer on hand that it can pass to AppendText and then pass eventually to io.Writer.Write.

@yerden
Copy link

yerden commented Sep 22, 2023

Upvoting. I cannot imagine any heavily loaded application tinkering with bytes and text that wouldn't implement an interface of similar semantics.

@mitar
Copy link
Contributor

mitar commented Oct 6, 2023

nor mutate any bytes within b[:len(b)].

Why, performance wise, would that not be allowed? Maybe we can open additional use cases if we do allow that?

@dsnet
Copy link
Member Author

dsnet commented Oct 6, 2023

Why, performance wise, would that not be allowed?

For a function named "Append", it just seems highly suspicious for it to mutate data within b[:len(b)]. The verb "append" implies only modifying data within b[len(b):cap(b)].

A performance argument could be made in the reverse. The caller of AppendText could in theory spin off a goroutine to do some form of read-only validation within b[:len(b)]. Mutating that section would result in a race condition.

@mitar
Copy link
Contributor

mitar commented Oct 6, 2023

For a function named "Append", it just seems highly suspicious for it to mutate data within b[:len(b)]. The verb "append" implies only modifying data within b[len(b):cap(b)].

So should we rename it then? Like MutateText(b []byte) ([]byte, error)?

A performance argument could be made in the reverse. The caller of AppendText could in theory spin off a goroutine to do some form of read-only validation within b[:len(b)]. Mutating that section would result in a race condition.

I think this is handled by the "Implementations must not retain b", if you make a goroutine, you are retaining b.

@dsnet
Copy link
Member Author

dsnet commented Oct 6, 2023

So should we rename it then?

I'm arguing that we should keep it named AppendText and also keep the documentation that implementations shouldn't mutate b[:len(b)].

I think this is handled by the "Implementations must not retain b", if you make a goroutine, you are retaining b.

In my example, the asynchronous validation logic is done by the caller of AppendText, not by the implementation of AppendText.

@mitar
Copy link
Contributor

mitar commented Oct 7, 2023

@dsnet Thanks for the explanation!

@mvdan
Copy link
Member

mvdan commented Apr 2, 2024

@ianlancetaylor could I nudge this proposal forward? It would help with the encoding/json/v2 efforts, and it seems to have reasonable consensus :)

@rsc
Copy link
Contributor

rsc commented Apr 24, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals Apr 24, 2024
gopherbot pushed a commit that referenced this issue Aug 21, 2024
Implement the encoding.(Binary|Text)Appender interfaces for "x509.OID".

Implement the encoding.BinaryAppender interface for "rand/v2.PCG" and "rand/v2.ChaCha8".

"rand/v2.ChaCha8.MarshalBinary" alse gains some performance benefits:

                           │     old      │                 new                 │
                           │    sec/op    │   sec/op     vs base                │
ChaCha8MarshalBinary-8       33.730n ± 2%   9.786n ± 1%  -70.99% (p=0.000 n=10)
ChaCha8MarshalBinaryRead-8    99.86n ± 1%   17.79n ± 0%  -82.18% (p=0.000 n=10)
geomean                       58.04n        13.19n       -77.27%

                           │    old     │                  new                   │
                           │    B/op    │   B/op     vs base                     │
ChaCha8MarshalBinary-8       48.00 ± 0%   0.00 ± 0%  -100.00% (p=0.000 n=10)
ChaCha8MarshalBinaryRead-8   83.00 ± 0%   0.00 ± 0%  -100.00% (p=0.000 n=10)

                           │    old     │                   new                   │
                           │ allocs/op  │ allocs/op   vs base                     │
ChaCha8MarshalBinary-8       1.000 ± 0%   0.000 ± 0%  -100.00% (p=0.000 n=10)
ChaCha8MarshalBinaryRead-8   2.000 ± 0%   0.000 ± 0%  -100.00% (p=0.000 n=10)

For #62384

Change-Id: I604bde6dad90a916012909c7260f4bb06dcf5c0a
GitHub-Last-Rev: 78abf9c
GitHub-Pull-Request: #68987
Reviewed-on: https://go-review.googlesource.com/c/go/+/607079
Reviewed-by: Cherry Mui <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
apocelipes added a commit to apocelipes/go that referenced this issue Aug 22, 2024
Implement the encoding.TextAppender for "net.IP".

Implament the encoding.(Binary|Text)Appender interfaces for
"netip.Addr", "netip.AddrPort" and "netip.Prefix".

All exported types in the standard library that implement the
"encoding.(Binary|Text)Marshaler" now also implement the
"encoding.(Binary|Text)Appender".

Fixes golang#62384
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/607520 mentions this issue: net,net/netip: implement the encoding.(Binary|Text)Appender

apocelipes added a commit to apocelipes/go that referenced this issue Aug 22, 2024
Implement the encoding.TextAppender for "net.IP".

Implament the encoding.(Binary|Text)Appender interfaces for
"netip.Addr", "netip.AddrPort" and "netip.Prefix".

All exported types in the standard library that implement the
"encoding.(Binary|Text)Marshaler" now also implement the
"encoding.(Binary|Text)Appender".

Fixes golang#62384
apocelipes added a commit to apocelipes/go that referenced this issue Aug 22, 2024
Implement the encoding.TextAppender interface for "net.IP".

Implament the encoding.(Binary|Text)Appender interfaces for
"netip.Addr", "netip.AddrPort" and "netip.Prefix".

All exported types in the standard library that implement the
"encoding.(Binary|Text)Marshaler" now also implement the
"encoding.(Binary|Text)Appender".

Fixes golang#62384
apocelipes added a commit to apocelipes/go that referenced this issue Aug 22, 2024
Implement the encoding.TextAppender interface for "net.IP".

Implement the encoding.(Binary|Text)Appender interfaces for
"netip.Addr", "netip.AddrPort" and "netip.Prefix".

"net.IP.MarshalText" gets some performance improvements:

                     │     old      │                 new                 │
                     │    sec/op    │   sec/op     vs base                │
IPMarshalText/IPv4-8    68.35n ± 1%   14.64n ± 1%  -78.57% (p=0.000 n=10)
IPMarshalText/IPv6-8   124.35n ± 2%   62.73n ± 1%  -49.55% (p=0.000 n=10)
geomean                 92.19n        30.31n       -67.12%

                     │    old     │                  new                   │
                     │    B/op    │   B/op     vs base                     │
IPMarshalText/IPv4-8   32.00 ± 0%   0.00 ± 0%  -100.00% (p=0.000 n=10)
IPMarshalText/IPv6-8   48.00 ± 0%   0.00 ± 0%  -100.00% (p=0.000 n=10)

                     │    old     │                   new                   │
                     │ allocs/op  │ allocs/op   vs base                     │
IPMarshalText/IPv4-8   2.000 ± 0%   0.000 ± 0%  -100.00% (p=0.000 n=10)
IPMarshalText/IPv6-8   2.000 ± 0%   0.000 ± 0%  -100.00% (p=0.000 n=10)

All exported types in the standard library that implement the
"encoding.(Binary|Text)Marshaler" now also implement the
"encoding.(Binary|Text)Appender".

Fixes golang#62384
apocelipes added a commit to apocelipes/go that referenced this issue Aug 22, 2024
Implement the encoding.TextAppender interface for "net.IP".

Implement the encoding.(Binary|Text)Appender interfaces for
"netip.Addr", "netip.AddrPort" and "netip.Prefix".

"net.IP.MarshalText" gets some performance improvements:

                     │     old      │                 new                 │
                     │    sec/op    │   sec/op     vs base                │
IPMarshalText/IPv4-8    68.35n ± 1%   14.64n ± 1%  -78.57% (p=0.000 n=10)
IPMarshalText/IPv6-8   124.35n ± 2%   62.73n ± 1%  -49.55% (p=0.000 n=10)
geomean                 92.19n        30.31n       -67.12%

                     │    old     │                  new                   │
                     │    B/op    │   B/op     vs base                     │
IPMarshalText/IPv4-8   32.00 ± 0%   0.00 ± 0%  -100.00% (p=0.000 n=10)
IPMarshalText/IPv6-8   48.00 ± 0%   0.00 ± 0%  -100.00% (p=0.000 n=10)

                     │    old     │                   new                   │
                     │ allocs/op  │ allocs/op   vs base                     │
IPMarshalText/IPv4-8   2.000 ± 0%   0.000 ± 0%  -100.00% (p=0.000 n=10)
IPMarshalText/IPv6-8   2.000 ± 0%   0.000 ± 0%  -100.00% (p=0.000 n=10)

All exported types in the standard library that implement the
"encoding.(Binary|Text)Marshaler" now also implement the
"encoding.(Binary|Text)Appender".

Fixes golang#62384
apocelipes added a commit to apocelipes/go that referenced this issue Sep 3, 2024
Implement the encoding.TextAppender interface for "net.IP".

Implement the encoding.(Binary|Text)Appender interfaces for
"netip.Addr", "netip.AddrPort" and "netip.Prefix".

"net.IP.MarshalText" gets some performance improvements:

                     │     old      │                 new                 │
                     │    sec/op    │   sec/op     vs base                │
IPMarshalText/IPv4-8    68.35n ± 1%   14.64n ± 1%  -78.57% (p=0.000 n=10)
IPMarshalText/IPv6-8   124.35n ± 2%   62.73n ± 1%  -49.55% (p=0.000 n=10)
geomean                 92.19n        30.31n       -67.12%

                     │    old     │                  new                   │
                     │    B/op    │   B/op     vs base                     │
IPMarshalText/IPv4-8   32.00 ± 0%   0.00 ± 0%  -100.00% (p=0.000 n=10)
IPMarshalText/IPv6-8   48.00 ± 0%   0.00 ± 0%  -100.00% (p=0.000 n=10)

                     │    old     │                   new                   │
                     │ allocs/op  │ allocs/op   vs base                     │
IPMarshalText/IPv4-8   2.000 ± 0%   0.000 ± 0%  -100.00% (p=0.000 n=10)
IPMarshalText/IPv6-8   2.000 ± 0%   0.000 ± 0%  -100.00% (p=0.000 n=10)

All exported types in the standard library that implement the
"encoding.(Binary|Text)Marshaler" now also implement the
"encoding.(Binary|Text)Appender".

Fixes golang#62384
apocelipes added a commit to apocelipes/go that referenced this issue Sep 19, 2024
Implement the encoding.TextAppender interface for "net.IP".

Implement the encoding.(Binary|Text)Appender interfaces for
"netip.Addr", "netip.AddrPort" and "netip.Prefix".

"net.IP.MarshalText" gets some performance improvements:

                     │     old      │                 new                 │
                     │    sec/op    │   sec/op     vs base                │
IPMarshalText/IPv4-8    68.35n ± 1%   14.64n ± 1%  -78.57% (p=0.000 n=10)
IPMarshalText/IPv6-8   124.35n ± 2%   62.73n ± 1%  -49.55% (p=0.000 n=10)
geomean                 92.19n        30.31n       -67.12%

                     │    old     │                  new                   │
                     │    B/op    │   B/op     vs base                     │
IPMarshalText/IPv4-8   32.00 ± 0%   0.00 ± 0%  -100.00% (p=0.000 n=10)
IPMarshalText/IPv6-8   48.00 ± 0%   0.00 ± 0%  -100.00% (p=0.000 n=10)

                     │    old     │                   new                   │
                     │ allocs/op  │ allocs/op   vs base                     │
IPMarshalText/IPv4-8   2.000 ± 0%   0.000 ± 0%  -100.00% (p=0.000 n=10)
IPMarshalText/IPv6-8   2.000 ± 0%   0.000 ± 0%  -100.00% (p=0.000 n=10)

All exported types in the standard library that implement the
"encoding.(Binary|Text)Marshaler" now also implement the
"encoding.(Binary|Text)Appender".

Fixes golang#62384
apocelipes added a commit to apocelipes/go that referenced this issue Sep 20, 2024
Implement the encoding.TextAppender interface for "net.IP".

Implement the encoding.(Binary|Text)Appender interfaces for
"netip.Addr", "netip.AddrPort" and "netip.Prefix".

"net.IP.MarshalText" gets some performance improvements:

                          │     old      │                 new                 │
                          │    sec/op    │   sec/op     vs base                │
IPMarshalText/IPv4-8         66.06n ± 1%   14.55n ± 1%  -77.97% (p=0.000 n=10)
IPMarshalText/IPv6-8        117.00n ± 1%   63.18n ± 1%  -46.00% (p=0.000 n=10)
IPMarshalText/IPv6_long-8    137.8n ± 1%   111.3n ± 1%  -19.27% (p=0.000 n=10)
geomean                      102.1n        46.77n       -54.21%

                          │    old     │                   new                   │
                          │    B/op    │    B/op     vs base                     │
IPMarshalText/IPv4-8        32.00 ± 0%    0.00 ± 0%  -100.00% (p=0.000 n=10)
IPMarshalText/IPv6-8        48.00 ± 0%    0.00 ± 0%  -100.00% (p=0.000 n=10)
IPMarshalText/IPv6_long-8   96.00 ± 0%   48.00 ± 0%   -50.00% (p=0.000 n=10)

                          │    old     │                   new                   │
                          │ allocs/op  │ allocs/op   vs base                     │
IPMarshalText/IPv4-8        2.000 ± 0%   0.000 ± 0%  -100.00% (p=0.000 n=10)
IPMarshalText/IPv6-8        2.000 ± 0%   0.000 ± 0%  -100.00% (p=0.000 n=10)
IPMarshalText/IPv6_long-8   2.000 ± 0%   1.000 ± 0%   -50.00% (p=0.000 n=10)

All exported types in the standard library that implement the
"encoding.(Binary|Text)Marshaler" now also implement the
"encoding.(Binary|Text)Appender".

Fixes golang#62384
@dmitshur dmitshur modified the milestones: Backlog, Go1.24 Nov 23, 2024
@aclements
Copy link
Member

I see these new methods are not being used nearly as widely as MarshalText/MarshalBinary in the standard library. Is anyone working on adding support for using these, particularly in encoding/json, encoding/xml, and encoding/gob?

@seankhliao
Copy link
Member

I'm not sure how useful this is for encoding/xml and encoding/json: These formats require escaping / transforming the string (insert backslashes, html escapes) before they can be added to the encoded state.

@dsnet
Copy link
Member Author

dsnet commented Dec 1, 2024

For "encoding/json", I propose we let the prospective future "encoding/json/v2" package be the one that introduces this optimization. Note that "encoding/json" will be implemented under-the-hood using "encoding/json/v2" and will implicitly pick up this optimization when that happens.

At present, I have no plans to implement this optimization in "encoding/xml" or "encoding/gob".

@dsnet
Copy link
Member Author

dsnet commented Dec 1, 2024

These formats require escaping / transforming the string (insert backslashes, html escapes) before they can be added to the encoded state.

True, but escaping is usually the exception, rather than the norm.

The v2 "json" package implements this by directly appending into the output buffer, and assumes that escaping is not necessary. If escaping is necessary, it copies to another intermediate buffer, and then escapes the text string back into the output buffer.

See https://github.com/go-json-experiment/json/blob/9802db03f36a812682e2bb94f58772d87e780d98/jsontext/encode.go#L450-L469

This optimization provided significant performance benefits at Tailscale since we serialize many netip.Addr and can now avoid both an allocation and string copy in the common case.

@aclements
Copy link
Member

For "encoding/json", I propose we let the prospective future "encoding/json/v2" package be the one that introduces this optimization.

Given that json/v2 is still in discussion, I'm not sure we should block on using this optimization in encoding/json.

The v2 "json" package implements this by directly appending into the output buffer, and assumes that escaping is not necessary. If escaping is necessary, it copies to another intermediate buffer, and then escapes the text string back into the output buffer.

Clever. Out of curiosity, why not do the escaping in place (using some backwards copying)? It seems like an intermediate buffer would at least sometimes require an allocation.

@dsnet
Copy link
Member Author

dsnet commented Dec 6, 2024

I'm not sure we should block on using this optimization in encoding/json.

We could, but even the few upstream changes that happened so far on "encoding/json" took me a while to rebase on top of my work to implement v1 in terms of v2. I'd prefer if v1 would stop changing otherwise I won't be able to catch up. Given that I'm not employed by Google, I only have spare cycles to work on v2.

Out of curiosity, why not do the escaping in place (using some backwards copying)?

We could, but that requires a specially written escape-in-place function, instead of re-using the existing append-and-escape function that already exists for other purposes.

@aclements
Copy link
Member

Thanks. One more thought as I go through the 1.24 API audit:

Should we give guidance on implementing and using this interfaces?

  • Should it be okay for a producer to implement only (Text|Binary)Appender and not (Text|Binary)Marshaler? Given that the new interfaces don't embed the old interface, this suggests that it is, but that may be a problem for consumers that don't support the new interfaces.

  • This is one of the first "upgrade" interfaces added since we added errors.ErrUnsupported. One intent with that error was that upgrade methods could use is as a sentinel error to indicate that, even though the type implements an interface, the caller should fall back to the old interface. This is important for wrapper types that don't statically know the interface supported by their wrappee. I suggest we should at least document this behavior for (Text|Binary)Appender.

@dsnet
Copy link
Member Author

dsnet commented Dec 8, 2024

Should it be okay for a producer to implement only (Text|Binary)Appender and not (Text|Binary)Marshaler?

Good question, I would argue on the basis of historical precedence that anything that implements the Appender should also implement the Marshaler for backwards compatibility. The Marshaler is a widely used interface that many libraries exist looking for that method, where most usages are not performance sensitive and would have little incentive to upgrade to support the Appender optimization.

This is one of the first "upgrade" interfaces added since we added errors.ErrUnsupported.

Another good question. If a wrapper types wants to implement Appender, I would argue that it should do the equivalent of:

func (v Wrapper) AppendText(b []byte) ([]byte, error) {
    if v2, ok := v.underlying.(encoding.TextAppender); ok {
        return v2.AppendText(b)
    }
    b2, err := v.MarshalText()
    return append(b, b2...), err
}

That is, there is no benefit to return errors.ErrUnsupported in the case where the underlying type does not implement TextAppender. Looking at this from the perspective of the caller, if AppendText reported ErrUnsupported, then the next logical action is to call MarshalText (which I argue in my previous point should always exist if AppendText exists). We might as well save the caller the trouble and do it on the caller's behalf. Thus, we would only get ErrUnsupported if MarshalText itself somehow reported the error. That said, we are now entering territory where the TextMarshaler interface provides no guidance on whether it can return ErrUnsupported (since it predated that sentinel error).

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/634515 mentions this issue: encoding/json: support TextAppender

@jellevandenhooff
Copy link
Contributor

jellevandenhooff commented Dec 9, 2024

I prototyped a change to encoding/json to use AppendText in https://go.dev/cl/634515. Like the change in https://github.com/go-json-experiment/json/blob/9802db03f36a812682e2bb94f58772d87e780d98/jsontext/encode.go#L450-L469, I used a temporary buffer to not need a new zero-copy escaper.

I like the description of the TextAppender API. It locks down behavior precisely and I think makes replacing MarshalText calls with AppendText safe and backwards compatible for libraries like encoding/json. I have two small concerns:

  • A type that implements TextAppender but not TextMarshaler is awkward as it will now marshal differently. I think it would be a good change, if rather last minute, to make TextAppender embed TextMarshaler. For TextAppender implementers this is little extra work, as MarshalText can simply be AppendText(nil).

  • With the TextAppender API a bad implementation could muck up earlier JSON output. With TextMarshaler that risk is much smaller. I accept the argument that bad implementations of io.Writer have the same problem today.

@aclements
Copy link
Member

Good question, I would argue on the basis of historical precedence that anything that implements the Appender should also implement the Marshaler for backwards compatibility.

Sounds good. We should document this on the Appender interfaces.

If a wrapper types wants to implement Appender, I would argue that it should do the equivalent of:

Good point. Given that this is an upgrade interface, I think we should provide functions that take care of this fallback, like:

package encoding

func AppendText(m TextMarshaler, b []byte) ([]byte, error) {
    if m2, ok := m.(encoding.TextAppender); ok {
        return m2.AppendText(b)
    }
    b2, err := m.MarshalText()
    return append(b, b2...), err
}

Obviously not for 1.24 at this point. I'm happy to file a little proposal for this when I have a chance, or feel free to yourself if you want.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/635336 mentions this issue: encoding: document that {Text,Binary}Appender implies {Text,Binary}Marshaler

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

Successfully merging a pull request may close this issue.