Permalink
Browse files

Only rewrite an image once even if it's requested via ipro many times

concurrently.

Print elapsed time properly for subshell tests. Fix typo in SVG
reference which was causing spurious timeout warnings in nginx tests,
and then flagged by nginx system tests.

Remove nginx test flakiness by using fetch_until an image gets small
to indicate an ipro-rewrite is done, rather than testing for
image_ongoing_rewrites to be zero. The old technique might fail because
we might check that stat before the image starts being rewritten.

Note: with this change, nginx system tests no longer flake for me,
whereas previously about 10% of the time it would flake on "IPRO flow
uses cache as expected". That was also suppressed for valgrind runs,
which is no longer needed.

Even after this change, nginx system tests with valgrind tests still flake
with "Fetch timed out" log messages, which I am adding to the
suppressions, and with "Embed image configuration in rewritten image
URL.", where the recursive-wget result is not optimized. I think that
might be a real user-facing bug, and I will report it to the nginx list.
  • Loading branch information...
jmarantz committed Jan 5, 2016
1 parent 83c9607 commit 62d24fbd88a5a7cb2405eded40ce4d9f20e1fc38
@@ -11,7 +11,7 @@
}
</style>
<body>
<img src="../../mod_pagespeed_example/images/Cschedule_event.svg"
<img src="../../mod_pagespeed_example/images/schedule_event.svg"
width="20" height="10"
title="SVG image won't be resized nor rewritten."/><br/>
<img src="../../mod_pagespeed_example/images/gray_saved_as_rgb.webp"
@@ -1708,7 +1708,7 @@ void RewriteContext::CallLockFailed() {
void RewriteContext::LockFailed() {
num_rewrites_abandoned_for_lock_contention_->Add(1);
MarkTooBusy();
Activate();
Finalize();
}

bool RewriteContext::IsDistributedRewriteForHtml() const {
@@ -237,7 +237,8 @@ function run_test() {
else
# Use a subshell to keep modifications tests make to the test environment
# from interfering with eachother.
(source "$this_dir/system_tests/${test_name}.sh")
(source "$this_dir/system_tests/${test_name}.sh"; update_elapsed_time)
previous_time_ms=0
fi
}

@@ -541,28 +542,33 @@ function get_stat() {
}

function check_stat() {
check_stat_op $1 $2 $3 $4 =
}

function check_stat_op() {
if [ "${statistics_enabled:-1}" -eq "0" ]; then
return
fi
local OLD_STATS_FILE=$1
local NEW_STATS_FILE=$2
local COUNTER_NAME=$3
local EXPECTED_DIFF=$4
local OP=$5
local OLD_VAL=$(get_stat ${COUNTER_NAME} <${OLD_STATS_FILE})
local NEW_VAL=$(get_stat ${COUNTER_NAME} <${NEW_STATS_FILE})

# This extra check is necessary because the syntax error in the second if
# does not cause bash to fail :/
if [ "${NEW_VAL}" != "" -a "${OLD_VAL}" != "" ]; then
if [ $((${NEW_VAL} - ${OLD_VAL})) = ${EXPECTED_DIFF} ]; then
if [ $((${NEW_VAL} - ${OLD_VAL})) $OP ${EXPECTED_DIFF} ]; then
return;
fi
fi

# Failure
local EXPECTED_VAL=$((${OLD_VAL} + ${EXPECTED_DIFF}))
echo -n "Mismatched counter value : ${COUNTER_NAME} : "
echo "Expected=${EXPECTED_VAL} Actual=${NEW_VAL}"
echo "Expected(${EXPECTED_VAL}) $OP Actual(${NEW_VAL})"
echo "Compare stat files ${OLD_STATS_FILE} and ${NEW_STATS_FILE}"
handle_failure
}
@@ -527,6 +527,19 @@ if [ $statistics_enabled = "1" ]; then
check [ $not_cacheable = $((not_cacheable_start + 1)) ]
URL=""
AUTH=""

start_test A burst of image requests should yield only one rewrite.
URL="$EXAMPLE_ROOT/images/Puzzle.jpg?a=$RANDOM"
start_image_rewrites=$(scrape_stat image_rewrites)
echo Running burst of 20x: \"wget -q -O - $URL '|' wc -c\"
for ((i = 0; i < 20; ++i)); do
echo -n $(wget -q -O - $URL | wc -c) ""
done
echo "... done"
sleep 1
end_image_rewrites=$(scrape_stat image_rewrites)
check [ $end_image_rewrites = $((start_image_rewrites + 1)) ]
URL=""
fi

if [ "$SECONDARY_HOSTNAME" != "" ]; then
@@ -1108,15 +1121,14 @@ if [ "$SECONDARY_HOSTNAME" != "" ]; then
check_stat $STATS.0 $STATS.1 image_rewrites 0

# Second IPRO request.
http_proxy=$SECONDARY_HOSTNAME check $WGET_DUMP $URL -O /dev/null
# Wait for image rewrite to finish.
http_proxy=$SECONDARY_HOSTNAME fetch_until "$IPRO_STATS_URL" \
'get_stat image_ongoing_rewrites' 0
# Original file has content-length 15131. Once ipro-optimized, it is
# 11395, so fetch it until it's less than 12000.
http_proxy=$SECONDARY_HOSTNAME fetch_until $URL "wc -c" 12000 "" "-lt"
http_proxy=$SECONDARY_HOSTNAME $WGET_DUMP $IPRO_STATS_URL > $STATS.2

# Resource is found in cache the second time.
check_stat $STATS.1 $STATS.2 cache_hits 1
check_stat $STATS.1 $STATS.2 ipro_served 1
check_stat_op $STATS.1 $STATS.2 cache_hits 1 -ge
check_stat_op $STATS.1 $STATS.2 ipro_served 1 -ge
check_stat $STATS.1 $STATS.2 ipro_not_rewritable 0
# So we don't run the ipro recorder flow.
check_stat $STATS.1 $STATS.2 ipro_not_in_cache 0

0 comments on commit 62d24fb

Please sign in to comment.