Hi Phil,
On Tue, Nov 05, 2024 at 06:07:50PM +0100, Phil Sutter wrote:
> On Tue, Nov 05, 2024 at 05:51:35PM +0100, Pablo Neira Ayuso wrote:
> > Hi Phil,
> >
> > On Tue, Nov 05, 2024 at 02:35:16PM +0100, Phil Sutter wrote:
> > > On Thu, Oct 31, 2024 at 11:04:11PM +0100, Pablo Neira Ayuso wrote:
> > > > Update json parser to collapse {add,create} element commands to reduce
> > > > memory consumption in the case of large sets defined by one element per
> > > > command:
> > > >
> > > > {"nftables": [{"add": {"element": {"family": "ip", "table": "x", "name":
> > > > "y", "elem": [{"set": ["1.1.0.0"]}]}}},...]}
> > >
> > > Thanks for the fix!
> > >
> > > > Add CTX_F_COLLAPSED flag to report that command has been collapsed.
> > >
> > > I had come up with a similar solution (but did not find time to submit
> > > it last week). My solution to the "what to return" problem was to
> > > introduce a 'static struct cmd cmd_nop' and return its address. Your
> > > flag way is fine, too from my PoV.
> >
> > OK, I'm going to push it out then.
> >
> > > > This patch reduces memory consumption by ~32% this case.
> > > >
> > > > Fixes: 20f1c60ac8c8 ("src: collapse set element commands from parser")
> > > > Reported-by: Eric Garver <eric@xxxxxxxxxxx>
> > > > Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> > > > ---
> > > > Side note: While profiling, I can still see lots json objects, this
> > > > results in memory consumption that is 5 times than native
> > > > representation. Error reporting is also lagging behind, it should be
> > > > possible to add a json_t pointer to struct location to relate
> > > > expressions and json objects.
> > >
> > > I can have a look at mem use if I find spare time (TM).
> >
> > I understand, that is always the issue.
>
> It's on my TODO at least, let's hope for the best.
Thanks, we are already discussing this in a different thread.
> > > We already record links between struct cmd and json_t objects for echo
> > > mode (and only then). The problem with error reporting in my opinion is
> > > the lack of location data in json_t. You might remember, I tried to
> > > extend libjansson to our needs but my MR[1] is being ignored for more
> > > than a year now. Should we just ship an extended copy in nftables?
> >
> > Do you still have the link with your proposal around? I don't find it
> > in my notes anymore.
>
> Ah, prolly forgot to resolve that [1] above:
>
> https://github.com/akheron/jansson/pull/662
>
> > IIRC the rejection came from concerns about increasing memory usage
> > for our specific usecase, that was an extra pointer to store location,
> > correct?
>
> That and lack of interest in the feature in general. See the linked !461
> for some feedback. The uncommented implementation in !662 hides
> everything behind a decoder flag and avoids any memory overhead if not
> enabled. The only remaining concern I can't address is: "we don't see
> this as an important feature that should be included in Jansson."
Your changeset for jansson look relative small, coding style look also
correct, and you have addressed the memory consumption issue.
It is a pity, I wonder what people do when json in this case? They
have to scavenge in the json soup to find for non-syntactic errors as
we have to do with it?
Having said this, I like jansson implementation, it is neat.
Caching a copy of libjansson in the nftables tree does not look like a
way to go IMO.
Is there a chance to poke them again / refresh your pull request?
Otherwise, we will have to search for alternative path to improve the
existing situation.
Thanks!
