user/dev discussion of public-inbox itself
 help / color / mirror / code / Atom feed
Search results ordered by [date|relevance]  view[summary|nested|Atom feed]
thread overview below | download mbox.gz: |
* Re: [PATCH 29/30] solvergit: allow passing arg to user-supplied callback
  2019-12-25  7:51  6% ` [PATCH 29/30] solvergit: allow passing arg to user-supplied callback Eric Wong
@ 2019-12-28  9:17  6%   ` Eric Wong
  0 siblings, 0 replies; 3+ results
From: Eric Wong @ 2019-12-28  9:17 UTC (permalink / raw)
  To: meta

Eric Wong <e@80x24.org> wrote:
> +++ b/lib/PublicInbox/SolverGit.pm

> @@ -524,7 +527,7 @@ sub resolve_patch ($$) {
>  			join("\n", $found_git->pub_urls($self->{psgi_env})));
>  
>  		if ($cur_want eq $self->{oid_want} || $type ne 'blob') {
> -			eval { delete($self->{user_cb})->($existing) };
> +			eval { done_err($self, $existing) };
>  			die "E: $@" if $@;
>  			return;
>  		}
> @@ -569,11 +572,12 @@ sub resolve_patch ($$) {
>  # this API is designed to avoid creating self-referential structures;
>  # so user_cb never references the SolverGit object

I missed this in resolve_patch:

	eval { delete($self->{user_cb})->(undef) }; # not found! :<

The following fixes it and adds a regression test:

We need to test the non-'0'x40 error path explicitly, since I
forgot to convert the {user_cb} invocation in resolve_patch()
to handle {uarg} :x.

diff --git a/lib/PublicInbox/SolverGit.pm b/lib/PublicInbox/SolverGit.pm
index 117040a0..17a43060 100644
--- a/lib/PublicInbox/SolverGit.pm
+++ b/lib/PublicInbox/SolverGit.pm
@@ -55,16 +55,16 @@ sub dbg ($$) {
 	print { $_[0]->{out} } $_[1], "\n" or ERR($_[0], "print(dbg): $!");
 }
 
-sub done_err ($$) {
-	my ($self, $err) = @_;
+sub done ($$) {
+	my ($self, $res) = @_;
 	my $ucb = delete($self->{user_cb}) or return;
-	$ucb->($err, $self->{uarg});
+	$ucb->($res, $self->{uarg});
 }
 
 sub ERR ($$) {
 	my ($self, $err) = @_;
 	print { $self->{out} } $err, "\n";
-	eval { done_err($self, $err) };
+	eval { done($self, $err) };
 	die $err;
 }
 
@@ -316,23 +316,23 @@ sub extract_old_mode ($) {
 	'100644';
 }
 
-sub do_finish ($$) {
-	my ($self, $user_cb) = @_;
-	my ($found, $oid_want, $uarg) = @$self{qw(found oid_want uarg)};
+sub do_finish ($) {
+	my ($self) = @_;
+	my ($found, $oid_want) = @$self{qw(found oid_want)};
 	if (my $exists = $found->{$oid_want}) {
-		return $user_cb->($exists, $uarg);
+		return done($self, $exists);
 	}
 
 	# let git disambiguate if oid_want was too short,
 	# but long enough to be unambiguous:
 	my $tmp_git = $self->{tmp_git};
 	if (my @res = $tmp_git->check($oid_want)) {
-		return $user_cb->($found->{$res[0]}, $uarg);
+		return done($self, $found->{$res[0]});
 	}
 	if (my $err = $tmp_git->last_check_err) {
 		dbg($self, $err);
 	}
-	$user_cb->(undef, $uarg);
+	done($self, undef);
 }
 
 sub event_step ($) {
@@ -356,8 +356,8 @@ sub event_step ($) {
 		# our result: (which may be undef)
 		# Other steps may call user_cb to terminate prematurely
 		# on error
-		} elsif (my $user_cb = delete($self->{user_cb})) {
-			do_finish($self, $user_cb);
+		} elsif (exists $self->{user_cb}) {
+			do_finish($self);
 		} else {
 			die 'about to call user_cb twice'; # Oops :x
 		}
@@ -366,7 +366,7 @@ sub event_step ($) {
 	if ($err) {
 		$err =~ s/^\s*Exception:\s*//; # bad word to show users :P
 		dbg($self, "E: $err");
-		eval { done_err($self, $err) };
+		eval { done($self, $err) };
 	}
 }
 
@@ -527,7 +527,7 @@ sub resolve_patch ($$) {
 			join("\n", $found_git->pub_urls($self->{psgi_env})));
 
 		if ($cur_want eq $self->{oid_want} || $type ne 'blob') {
-			eval { done_err($self, $existing) };
+			eval { done($self, $existing) };
 			die "E: $@" if $@;
 			return;
 		}
@@ -565,7 +565,7 @@ sub resolve_patch ($$) {
 	}
 
 	dbg($self, "could not find $cur_want");
-	eval { delete($self->{user_cb})->(undef) }; # not found! :<
+	eval { done($self, undef) };
 	die "E: $@" if $@;
 }
 
@@ -595,7 +595,7 @@ sub solve ($$$$$) {
 
 	# should we even get here? Probably not, but somebody
 	# could be manually typing URLs:
-	return done_err($self, undef) if $oid_want =~ /\A0+\z/;
+	return done($self, undef) if $oid_want =~ /\A0+\z/;
 
 	$self->{oid_want} = $oid_want;
 	$self->{out} = $out;
diff --git a/t/solver_git.t b/t/solver_git.t
index af5bf7bc..0c272d77 100644
--- a/t/solver_git.t
+++ b/t/solver_git.t
@@ -157,6 +157,7 @@ EOF
 	close $cfgfh or die;
 	my $cfg = PublicInbox::Config->new($cfgpath);
 	my $www = PublicInbox::WWW->new($cfg);
+	my $non_existent = 'ee5e32211bf62ab6531bdf39b84b6920d0b6775a';
 	my $client = sub {
 		my ($cb) = @_;
 		my $res = $cb->(GET("/$name/3435775/s/"));
@@ -165,6 +166,9 @@ EOF
 		$res = $cb->(GET("/$name/".('0'x40).'/s/'));
 		is($res->code, 404, 'failure with null OID');
 
+		$res = $cb->(GET("/$name/$non_existent/s/"));
+		is($res->code, 404, 'failure with null OID');
+
 		$res = $cb->(GET("/$name/$v1_0_0_tag/s/"));
 		is($res->code, 200, 'shows commit');
 		while (my ($label, $size) = each %bin) {

^ permalink raw reply related	[relevance 6%]

* [PATCH 29/30] solvergit: allow passing arg to user-supplied callback
  2019-12-25  7:50  7% [PATCH 00/30] www: eliminate most per-request closures Eric Wong
@ 2019-12-25  7:51  6% ` Eric Wong
  2019-12-28  9:17  6%   ` Eric Wong
  0 siblings, 1 reply; 3+ results
From: Eric Wong @ 2019-12-25  7:51 UTC (permalink / raw)
  To: meta

This allows us to get rid of the requirement to capture
on-stack variables with an anonymous sub, as illustrated
with the update to viewvcs to take advantage of this.
---
 lib/PublicInbox/SolverGit.pm | 28 ++++++++++++++++------------
 lib/PublicInbox/ViewVCS.pm   | 19 ++++++++++---------
 2 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/lib/PublicInbox/SolverGit.pm b/lib/PublicInbox/SolverGit.pm
index 6dfa20f7..6b6586e9 100644
--- a/lib/PublicInbox/SolverGit.pm
+++ b/lib/PublicInbox/SolverGit.pm
@@ -55,11 +55,16 @@ sub dbg ($$) {
 	print { $_[0]->{out} } $_[1], "\n" or ERR($_[0], "print(dbg): $!");
 }
 
+sub done_err ($$) {
+	my ($self, $err) = @_;
+	my $ucb = delete($self->{user_cb}) or return;
+	$ucb->($err, $self->{uarg});
+}
+
 sub ERR ($$) {
 	my ($self, $err) = @_;
 	print { $self->{out} } $err, "\n";
-	my $ucb = delete($self->{user_cb});
-	eval { $ucb->($err) } if $ucb;
+	eval { done_err($self, $err) };
 	die $err;
 }
 
@@ -313,22 +318,21 @@ sub extract_old_mode ($) {
 
 sub do_finish ($$) {
 	my ($self, $user_cb) = @_;
-	my $found = $self->{found};
-	my $oid_want = $self->{oid_want};
+	my ($found, $oid_want, $uarg) = @$self{qw(found oid_want uarg)};
 	if (my $exists = $found->{$oid_want}) {
-		return $user_cb->($exists);
+		return $user_cb->($exists, $uarg);
 	}
 
 	# let git disambiguate if oid_want was too short,
 	# but long enough to be unambiguous:
 	my $tmp_git = $self->{tmp_git};
 	if (my @res = $tmp_git->check($oid_want)) {
-		return $user_cb->($found->{$res[0]});
+		return $user_cb->($found->{$res[0]}, $uarg);
 	}
 	if (my $err = $tmp_git->last_check_err) {
 		dbg($self, $err);
 	}
-	$user_cb->(undef);
+	$user_cb->(undef, $uarg);
 }
 
 sub do_step ($) {
@@ -362,8 +366,7 @@ sub do_step ($) {
 	if ($err) {
 		$err =~ s/^\s*Exception:\s*//; # bad word to show users :P
 		dbg($self, "E: $err");
-		my $ucb = delete($self->{user_cb});
-		eval { $ucb->($err) } if $ucb;
+		eval { done_err($self, $err) };
 	}
 }
 
@@ -524,7 +527,7 @@ sub resolve_patch ($$) {
 			join("\n", $found_git->pub_urls($self->{psgi_env})));
 
 		if ($cur_want eq $self->{oid_want} || $type ne 'blob') {
-			eval { delete($self->{user_cb})->($existing) };
+			eval { done_err($self, $existing) };
 			die "E: $@" if $@;
 			return;
 		}
@@ -569,11 +572,12 @@ sub resolve_patch ($$) {
 # this API is designed to avoid creating self-referential structures;
 # so user_cb never references the SolverGit object
 sub new {
-	my ($class, $ibx, $user_cb) = @_;
+	my ($class, $ibx, $user_cb, $uarg) = @_;
 
 	bless {
 		gits => $ibx->{-repo_objs},
 		user_cb => $user_cb,
+		uarg => $uarg,
 		# -cur_di, -qsp, -msg => temporary fields for Qspawn callbacks
 
 		# TODO: config option for searching related inboxes
@@ -591,7 +595,7 @@ sub solve ($$$$$) {
 
 	# should we even get here? Probably not, but somebody
 	# could be manually typing URLs:
-	return (delete $self->{user_cb})->(undef) if $oid_want =~ /\A0+\z/;
+	return done_err($self, undef) if $oid_want =~ /\A0+\z/;
 
 	$self->{oid_want} = $oid_want;
 	$self->{out} = $out;
diff --git a/lib/PublicInbox/ViewVCS.pm b/lib/PublicInbox/ViewVCS.pm
index a6dbb9a9..ead8c2b4 100644
--- a/lib/PublicInbox/ViewVCS.pm
+++ b/lib/PublicInbox/ViewVCS.pm
@@ -112,8 +112,10 @@ sub show_other ($$$$) {
 	$qsp->psgi_qx($env, undef, \&show_other_result, $ctx);
 }
 
+# user_cb for SolverGit, called as: user_cb->($result_or_error, $uarg)
 sub solve_result {
-	my ($ctx, $res, $log, $hints, $fn) = @_;
+	my ($res, $ctx) = @_;
+	my ($log, $hints, $fn) = delete @$ctx{qw(log hints fn)};
 
 	unless (seek($log, 0, 0)) {
 		$ctx->{env}->{'psgi.errors'}->print("seek(log): $!\n");
@@ -192,21 +194,20 @@ sub solve_result {
 sub show ($$;$) {
 	my ($ctx, $oid_b, $fn) = @_;
 	my $qp = $ctx->{qp};
-	my $hints = {};
+	my $hints = $ctx->{hints} = {};
 	while (my ($from, $to) = each %QP_MAP) {
 		defined(my $v = $qp->{$from}) or next;
 		$hints->{$to} = $v;
 	}
 
-	my $log = tmpfile("solve.$oid_b");
-	my $solver = PublicInbox::SolverGit->new($ctx->{-inbox}, sub {
-		solve_result($ctx, $_[0], $log, $hints, $fn);
-	});
-
-	# PSGI server will call this and give us a callback
+	$ctx->{'log'} = tmpfile("solve.$oid_b");
+	$ctx->{fn} = $fn;
+	my $solver = PublicInbox::SolverGit->new($ctx->{-inbox},
+						\&solve_result, $ctx);
+	# PSGI server will call this immediately and give us a callback (-wcb)
 	sub {
 		$ctx->{-wcb} = $_[0]; # HTTP write callback
-		$solver->solve($ctx->{env}, $log, $oid_b, $hints);
+		$solver->solve($ctx->{env}, $ctx->{log}, $oid_b, $hints);
 	};
 }
 

^ permalink raw reply related	[relevance 6%]

* [PATCH 00/30] www: eliminate most per-request closures
@ 2019-12-25  7:50  7% Eric Wong
  2019-12-25  7:51  6% ` [PATCH 29/30] solvergit: allow passing arg to user-supplied callback Eric Wong
  0 siblings, 1 reply; 3+ results
From: Eric Wong @ 2019-12-25  7:50 UTC (permalink / raw)
  To: meta

Closures (aka "anonymous subs") tack several KB of memory onto
every WWW request/response, decreasing scalability and
performance of our WWW endpoints.  They also increase human
review time to check for reference cycles.

Similar changes to -nntpd and the generic parts of -httpd were
also done recently:
https://public-inbox.org/meta/20191221235319.27082-1-e@80x24.org/
https://public-inbox.org/meta/20191221080007.27810-1-e@80x24.org/

These could still use some naming improvements, and it's been
pretty tiring writing the same-ish commit message over and over.

All these changes around eliminating closures also make it
easier to port our codebase to languages which lack closures.

Fwiw, I've been brainstorming ideas to create a new, refcounted
language where cyclic references are impossible by design.  Such
a design would not be possible if closures were implemented; but
doable otherwise by taking a hint from *nix FSes.

Eric Wong (30):
  git: allow async_cat to pass arg to callback
  httpd/async: support passing arg to callbacks
  qspawn: remove some anonymous subs for psgi_qx
  qspawn: disambiguate command vs PSGI env
  qspawn: replace anonymous $end callbacks w/ event_step
  msg_iter: provide means to stop using anonymous subs
  qspawn: reduce local vars, de-anonymize rd_hdr
  httpd/async: get rid of ephemeral main_cb
  qspawn: psgi_return: initial cb can be named
  qspawn: psgi_return_start: hoist out from psgi_return
  qspawn: psgi_qx: eliminate anonymous subs
  qspawn: drop "qspawn.filter" support, for now
  qspawn: psgi_return: allow non-anon parse_hdr callback
  githttpbackend: split out wwwstatic
  www: lazy load Plack::Util
  mboxgz: pass $ctx to callback to avoid anon subs
  feed: avoid anonymous subs
  config: each_inbox: pass user arg to callback
  view: avoid anon sub in stream_thread
  view: msg_html: stop using an anonymous sub
  contentid: no anonymous sub
  wwwtext: avoid anonymous sub in response
  searchview: pass named subs to Www*Stream
  view: thread_html: pass named sub to WwwStream
  searchview: remove anonymous sub when sorting threads by relevance
  view: msg_iter calls add_body_text directly
  wwwattach: avoid anonymous sub for msg_iter
  viewvcs: avoid anonymous sub for HTML response
  solvergit: allow passing arg to user-supplied callback
  search: retry_reopen passes user arg to callback

 MANIFEST                          |   1 +
 lib/PublicInbox/Cgit.pm           |  19 +-
 lib/PublicInbox/Config.pm         |  11 +-
 lib/PublicInbox/ContentId.pm      |  53 +++---
 lib/PublicInbox/ExtMsg.pm         |  58 +++---
 lib/PublicInbox/Feed.pm           |  51 +++--
 lib/PublicInbox/GetlineBody.pm    |  12 +-
 lib/PublicInbox/Git.pm            |  14 +-
 lib/PublicInbox/GitHTTPBackend.pm |  99 +---------
 lib/PublicInbox/HTTPD/Async.pm    |  56 +++---
 lib/PublicInbox/Mbox.pm           | 131 +++++++------
 lib/PublicInbox/MboxGz.pm         |   2 +-
 lib/PublicInbox/MsgIter.pm        |   8 +-
 lib/PublicInbox/NewsWWW.pm        |  16 +-
 lib/PublicInbox/Qspawn.pm         | 296 +++++++++++++++---------------
 lib/PublicInbox/Search.pm         |  16 +-
 lib/PublicInbox/SearchMsg.pm      |   9 +-
 lib/PublicInbox/SearchView.pm     | 100 +++++-----
 lib/PublicInbox/SolverGit.pm      | 149 ++++++++-------
 lib/PublicInbox/View.pm           | 187 ++++++++++---------
 lib/PublicInbox/ViewVCS.pm        | 111 ++++++-----
 lib/PublicInbox/WWW.pm            |   2 +-
 lib/PublicInbox/WwwAtomStream.pm  |   2 +-
 lib/PublicInbox/WwwAttach.pm      |  49 ++---
 lib/PublicInbox/WwwListing.pm     |  37 ++--
 lib/PublicInbox/WwwStatic.pm      | 105 +++++++++++
 lib/PublicInbox/WwwText.pm        |  20 +-
 t/git.t                           |  21 +++
 t/qspawn.t                        |  19 +-
 29 files changed, 882 insertions(+), 772 deletions(-)
 create mode 100644 lib/PublicInbox/WwwStatic.pm


^ permalink raw reply	[relevance 7%]

Results 1-3 of 3 | reverse | options above
-- pct% links below jump to the message on this page, permalinks otherwise --
2019-12-25  7:50  7% [PATCH 00/30] www: eliminate most per-request closures Eric Wong
2019-12-25  7:51  6% ` [PATCH 29/30] solvergit: allow passing arg to user-supplied callback Eric Wong
2019-12-28  9:17  6%   ` Eric Wong

Code repositories for project(s) associated with this public inbox

	https://80x24.org/public-inbox.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).