Re: [PATCH v2 08/15] config: introduce an optional event stream while parsing

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 9, 2018, 07:35 UTC
Message-ID: <nycvar.QRO.7.76.6.1804090924060.55@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>
In-Reply-To: <20180406212227.GA7870@sigill.intra.peff.net>

Hi Peff,

On Fri, 6 Apr 2018, Jeff King wrote:

Show 13 quoted lines

> On Tue, Apr 03, 2018 at 06:28:29PM +0200, Johannes Schindelin wrote:
> 
> > This extends our config parser so that it can optionally produce an
> > event stream via callback function, where it reports e.g. when a
> > comment was parsed, or a section header, etc.
> > 
> > This parser will be used subsequently to handle the scenarios better
> > where removing config entries would make sections empty, or where a
> > new entry could be added to an already-existing, empty section.
> 
> Nice, it looks like this didn't end up being too bad to go in this
> direction. It seems like this is an optional "also emit the events here"
> function you can set.

Yes.

> I think in the long run we could actually just always emit the events to
> this function. And then we could wrap that to provide an interface that
> matches the existing callbacks (just an event-stream callback that sees
> EVENT_ENTRY and calls the sub-callback).

Well, not precisely. The event stream was implemented in a minimal fashion, in particular *not* emitting enough information in the event stream for that. To keep things as little intrusive as possible, the CONFIG_EVENT_ENTRY event is only emitted *after* the config_fn is called, and at that point we do not even know the key and the value any more.

I fear that it would make the code quite a bit more complicated to change it in the way you suggested.

Side note: a slightly ugly aspect of my patch series is that the CONFIG_EVENT_SECTION event *also* does not provide the interesting information (in this case, the section name), but that it has to be inferred from the cf->var field (which is file-local to config.c, and which has been set to the section name followed by a single '.' at that point). Again, this keeps the diff simpler to review, and that's why I did it that way.

> But that might end up quite a pain, since we have a zillion entry points
> into the config parser, making wrapping tough. So I'm perfectly happy to
> stop here for now.

Right.

Show 6 quoted lines

> > +static inline int do_event(enum config_event_t type,
> > +			   struct parse_event_data *data)
> 
> I'm not sure if "inline" here is a good idea, as it seems to get called
> quite a few times. If we're trying to make things fast, bloating the
> instruction cache may have the opposite effect.

Good point.

The reason I declared this as inline function was that I test whether either data->opts or data->opts->event_fn are NULL, and whether we are continuing to look at whitespace, for early returns from that function. Which I wanted to avoid doing in a hot function (I'd rather skip calling the function if it is pointless to call it).

However, the config code is hardly performance-critical, as we do not expect to parse hundreds of kilobytes, right? So that "inline" was a premature optimization.

Thanks, Dscho