Add mark/reset support for TranslatorSupport

Justin Deoliveira added a comment - 01/May/12 2:57 PM Looking good David, a couple of things: 1. no test cases 2. no need for exclamation ponts in exception messages...i think the java stack trace is jarring enough for users :) Also something to be weary of is performance. Those methods start,chars,etc... tend to be called a lot during say gml generation of large datasets. It would be good to see if there are any performance degradation that occurs here by adding the conditionals to all the methods. Could be an informal benchmark but it would be good to say take a dataset of a few million features and do a gml dump via geoserver of them... measuring pre and post patch to see if any degradation occurs.

David Winslow added a comment - 01/May/12 4:37 PM - edited 1) Yes, I'll add some test cases. 2) Ok, no problem there, I'll remove the exclamation points. 3) WRT performance - I would expect most uses of TranslatorSupport to be IO-bound rather than CPU-bound, and indeed didn't do any performance testing before posting this patch. I just ran a few quick checks: a) Generating an xml document like `<integers><integer>1</integer><integer>2</integer>...</integers>` and sending it to `/dev/null` The idea here is to avoid IO as much as possible and get an idea of the "pure" CPU cost. After a few tests I get about 0.95s to encode 1 million integers with the old encoder and about 1.23s to encode 1 million integers with the new one - about a 30% performance cost. b) "Real world" encoding of data from Shapefile through GeoServer. I don't have any million-feature datasets handy, but I did a test with a polygon layer I have lying around with many attributes and about 100000 features. Without guards, GeoServer took about 53.2s to write out GML for this layer, and 57.5s with the guards. Here we're seeing about an 8% performance cost. I imagine if we got into larger datasets that don't fit in memory we'd see even less of an impact. What do you think? Is 8% too much of a slowdown? I actually started to implement this functionality as a separate subclass of TranslatorSupport but it seems to me that even that would require modifying TranslatorSupport, so I went with the patch as written here since it's less code.

David Winslow added a comment - 02/May/12 2:35 PM 0002...patch adds unit tests (and removes the bang!s) mark-reset.patch is a cumulative patch for convenience in applying.

Chris Holmes added a comment - 03/May/12 5:48 AM A couple thoughts / questions on performance. In general I think an 8% slowdown isn't desirable, unless it has some very big win. I see the big win the Capabilities generation - misconfiguration completely breaking the caps document and thus many external applications is a constant problem that causes really bad things right now. So an 8% hit in that seems totally worth it. I don't yet see the win in GML output. I guess maybe I've just seen less errors there, especially that really affect people. If we're putting in extra guards for use cases that are rarely hit then an 8% performance hit seems like a lot. Lots of people like to compare and 'measure' us based on performance, and it'd be good to do as well as possible there. Especially if there's not a big win in terms of error reduction, etc. Would there be a way for the GML output to call the xml output without the mark/reset and not have the performance degradation, but have the capabilities call it?

David Winslow added a comment - 03/May/12 9:57 AM Rewrite allowing xml encoders to "opt out" of mark/reset support for performance

David Winslow added a comment - 03/May/12 10:02 AM Just to clarify: That 8% isn't a very scientific number, as I was doing some more tests today i saw variation of around 4% just with the original code (no buffering juju involved.) However, my original mark/reset implementation imposed an overhead for ALL XML writers, but required modifications to calling code to actually benefit from the new functionality - if an encoder doesn't call the mark method then it will never actually use buffer, but every XML write operation was checking a flag to see whether it should be buffered or not. Anyway, I've just attached a rewrite that moves all the buffering implementation into a separate base class, so encoders that don't use the buffered output mode don't invoke any of the buffering machinery. I also switched to a more object-oriented approach for the buffered implementation, so the cost of using it is much lower (I wasn't able to measure it with my dataset, but I'm not planning to spend more time trying to benchmark since it's opt-in now anyway.)

Andrea Aime added a comment - 04/May/12 1:17 AM What if anything in "commit" fails? Maybe better to have a try/finally there and switch to the direct backend anyways? Probably really minor thing, since once a failure has been thrown the whole operation will be aborted, but still...

David Winslow added a comment - 10/May/12 9:33 AM I think it's not that important to deal with "after" a commit fails - at that point you have no information on how much writing happened, whether the IO problem is transient, etc. so it's not safe to keep using the translator anyway. So I don't think it matters how we deal with the exception there. Obviously I can make this change if you guys feel that it should block bringing this patch in. (BTW: I am not a committer so I'll need someone else to apply it for me.)

Andrea Aime added a comment - 10/May/12 9:59 AM Ah ok, so the patches are ready for commit? I cannot do it before the weekend, added Justin as a watcher, maybe he can do that before

David Winslow added a comment - 10/May/12 10:04 AM Yes, I think these patches are ready for commit. I have verified that they meet my needs for the GeoServer feature they are intended to support. Only the final "mark-reset2.patch" attachment should be applied (it replaces, not adds onto, the others.)